-
Notifications
You must be signed in to change notification settings - Fork 100
Adds basic Spring Data Envers specific documentation. #279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Paging @Buzzardo |
src/main/asciidoc/envers.adoc
Outdated
| == Getting Started | ||
|
|
||
| As a starting point for using Spring Data Envers you need a project with Spring Data JPA. | ||
| The easiest way to create that is using the Spring Initializr either on start.spring.io or via its integration in your favorite IDE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Link to start.spring.io
src/main/asciidoc/envers.adoc
Outdated
|
|
||
| </dependencies> | ||
| ---- | ||
| This will also bring hibernate-envers into the project as a transient dependency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Code fences for hibernate-envers
src/main/asciidoc/envers.adoc
Outdated
| ---- | ||
| public interface PersonRepository | ||
| extends CrudRepository<Person, Long>, | ||
| RevisionRepository<Person, Long, Long> {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to add callouts and explain Person, Long, Long. It's not obvious why there's Long, Long.
src/main/asciidoc/envers.adoc
Outdated
| public class EnversDemoConfiguration {} | ||
| ---- | ||
|
|
||
| The entity for that repository should be an entity with Envers auditing enabled, i.e. it has an `@Audited` annotation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should or is this a hard requirement?
src/main/asciidoc/envers.adoc
Outdated
| [source,java] | ||
| ---- | ||
| @ExtendWith(SpringExtension.class) | ||
| @SpringBootTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rewrite the test to not require Spring Boot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that mean we have to reference an application context configuration which then should also be included in the example? Or are you aiming for something else?
src/main/asciidoc/envers.adoc
Outdated
|
|
||
| Revisions<Long, Person> revisions = repository.findRevisions(updated.id); | ||
|
|
||
| assertThat(revisions) // |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The // are rather distracting. Also, let's rewrite the test to a more simple form. extracting(…) and tuple aren't quite what we should expect from our audience as pre-requisite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully agree on the //
But I consider AssertJ the de facto standard of assertions and extracting + tuple essential and easy to understand features. Rewriting this would make it harder to understand AND enforces bad practices. I'm somewhat reluctant to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My motivation is that our example should be focusing on our bits/Envers bits as much as possible. The tests should just decorate the example and require as little knowledge as possible as the cognitive load to understand how to use Revisions is quite high for someone that wants to learn the topic. We should reduce the impact of our decoration on the learning steps.
| john.setName("John"); | ||
|
|
||
| //create | ||
| Person saved = tx.execute(__ -> repository.save(john)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's replace __ with transactionStatus for a more expressive example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again I don't find that more expressive but unnecessary verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's more verbose it avoids confusion with Kotlin or Scala-like implicit lambda parameters and it's not implying that any potentially newer and lesser-known feature is used. That being said, trying to remove obstacles.
Closes #61 See spring-projects/spring-data-examples#603 Original pull request: #279.
Closes #61 See spring-projects/spring-data-examples#603 Original pull request: #279.
Closes #61 See spring-projects/spring-data-examples#603 Original pull request: #279.
|
That's merged, polished, and backported now. |
Closes #61
See spring-projects/spring-data-examples#603