Skip to content

Conversation

Buzzardo
Copy link

@Buzzardo Buzzardo commented Apr 5, 2018

I edited for spelling, punctuation, grammar, and corporate voice. I also added cross-references in a few places.

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).
@mp911de
Copy link
Member

mp911de commented Apr 10, 2018

Thanks a lot. I filed DATACMNS-1291 to track this pull request.

Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the editing. Doing a full pass on the documentation is a valuable exercise and I found some spots we need to add further documentation or align wording. Documentation reads more polished now.

I left a few comments (some as reminders for me) on the pull request.

====

As you can see, the annotations can be applied selectively, depending on which information you'd like to capture. For the annotations capturing the points in time can be used on properties of type JodaTimes `DateTime`, legacy Java `Date` and `Calendar`, JDK8 date/time types as well as `long`/`Long`.
As you can see, the annotations can be applied selectively, depending on which information you want to capture. The annotations capturing when changes were made can be used on properties of type `JodaTimes`, `DateTime`, legacy Java `Date` and `Calendar`, JDK8 date and time types, and `long` or `Long`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JodaTime is a library. Maybe we should state it as Joda-Time (that's how they write it on http://www.joda.org/joda-time/)?

Example: "… can be used on properties of type Joda-Time, DateTime, …"

== Introduction

This chapter will give you an introduction to Query by Example and explain how to use Examples.
This chapter provides an introduction to Query by Example and explains how to use the Examples.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example in Query-by-Example is not specific to code samples here but rather a concept. Is it still "use the Examples"? Or should we rather rewrite the last part to something like "and explains how to use it."?

====

By default the `ExampleMatcher` will expect all values set on the probe to match. If you want to get results matching any of the predicates defined implicitly, use `ExampleMatcher.matchingAny()`.
By default, the `ExampleMatcher` expect alls values set on the probe to match. If you want to get results matching any of the predicates defined implicitly, use `ExampleMatcher.matchingAny()`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it alls or all?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was aiming for "expects all" and missed. Thanks for catching it. :)

====

NOTE: We also provide persistence technology-specific abstractions like e.g. `JpaRepository` or `MongoRepository`. Those interfaces extend `CrudRepository` and expose the capabilities of the underlying persistence technology in addition to the rather generic persistence technology-agnostic interfaces like e.g. CrudRepository.
NOTE: We also provide persistence technology-specific abstractions, such as `JpaRepository` or `MongoRepository`. Those interfaces extend `CrudRepository` and expose the capabilities of the underlying persistence technology in addition to the rather generic persistence technology-agnostic interfaces such as `CrudRepository`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We should clean up the superfluous whitespaces right after extend.

====

Accessing the second page of `User` by a page size of 20 you could simply do something like this:
To access the second page of `User` by a page size of 20, you could do something like the following:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We should clean up the superfluous whitespaces between could and do.

=== Web support

NOTE: This section contains the documentation for the Spring Data web support as it is implemented as of Spring Data Commons in the 1.6 range. As it the newly introduced support changes quite a lot of things we kept the documentation of the former behavior in <<web.legacy>>.
NOTE: This section contains the documentation for the Spring Data web support as it is implemented in the Spring Data Commons version (and later versions). As the newly introduced support changes many things, we kept the documentation of the former behavior in <<web.legacy>>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about …it is implemented in the current Spring Data Commons version.?

==== Web Databinding Support

Spring Data projections – generally described in <<projections>>can be used to bind incoming request payloads by either using http://goessner.net/articles/JsonPath/[JSONPath] expressions (requires https://github.com/json-path/JsonPath[Jayway JasonPath] or https://www.w3.org/TR/xpath-31/[XPath] expressions (requires https://xmlbeam.org/[XmlBeam]).
Spring Data projections (described in <<projections>>) can be used to bind incoming request payloads by either using http://goessner.net/articles/JsonPath/[JSONPath] expressions (requires https://github.com/json-path/JsonPath[Jayway JasonPath] or https://www.w3.org/TR/xpath-31/[XPath] expressions (requires https://xmlbeam.org/[XmlBeam]), as shown in the following example:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JasonPath -> JsonPath


[[web.legacy]]
=== Legacy web support
=== Legacy Web Support
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether we should remove this section as we have older version documentation available at docs.spring.io.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very much agreed. I removed it.

|`Optional<T>`|A Java 8 or Guava `Optional`. Expects the query method to return one result at most. In case no result is found `Optional.empty()`/`Optional.absent()` is returned. More than one result will trigger an `IncorrectResultSizeDataAccessException`.
|`Option<T>`|An either Scala or JavaSlang `Option` type. Semantically same behavior as Java 8's `Optional` described above.
|`Optional<T>`|A Java 8 or Guava `Optional`. Expects the query method to return one result at most. If no result is found, `Optional.empty()` or `Optional.absent()` is returned. More than one result triggers an `IncorrectResultSizeDataAccessException`.
|`Option<T>`|Either a Scala or JavaSlang `Option` type. Semantically the same behavior as Java 8's `Optional`, described earlier.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Align JavaSlang to Javaslang (as mentioned in an earlier chapter)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Thanks. (I checked, and it's Javaslang).

|`Page<T>`|A `Slice` with additional information, such as the total number of results. Requires a `Pageable` method parameter.
|`GeoResult<T>`|A result entry with additional information, such as the distance to a reference location.
|`GeoResults<T>`|A list of `GeoResult<T>` with additional information, such as the average distance to a reference location.
|`GeoPage<T>`|A `Page` with `GeoResult<T>`, such as the average distance to a reference location.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder for @mp911de: We should add RxJava 1/2 and Project Reactor types.

@mp911de
Copy link
Member

mp911de commented Apr 12, 2018

Awesome, thank you so much. I'll push a commit on your branch addressing the issues I found while going over the existing docs so we can have another review pass by someone else from the Data team.

Copy link
Member

@odrotbohm odrotbohm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about the additions of these static resources in src/main/asciidoc as it effectively means we have to replicate that into all other Spring Data modules.

I'd much more prefer to have them in https://github.com/spring-projects/spring-data-build/tree/master/resources so that the projects can refer to them through the build and we have a canonical place to make changes to all of the docs.

pom.xml Outdated
<groupId>org.springframework.data.build</groupId>
<artifactId>spring-data-parent</artifactId>
<version>2.1.0.BUILD-SNAPSHOT</version>
<relativePath>../spring-data-build/parent/pom.xml</relativePath>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason we add this? We usually don't use relative paths to make sure the lookup of the parent is always done from the repository. Also, the declaration assumes that the parent project has been checked out into a folder of that exact name, which might not be correct. Also, if that folder has the project checked out in a different branch, it might see a different pom.

@@ -0,0 +1,51 @@
<script type="text/javascript" src="tocbot-3.0.2/tocbot.js"></script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these additions (also docinfo.html below) rather be done in the parent project and be consumed by the build? I'd want to avoid having to add these files to all Spring Data projects as that makes changes to them cumbersome.

@@ -0,0 +1,15 @@
<style>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment for docinfo-footer.html.

Oliver Gierke; Thomas Darimont; Christoph Strobl; Mark Pollack; Thomas Risberg; Mark Paluch; Jay Bryant
:revnumber: {version}
:revdate: {localdate}
:linkcss:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, I've preferred to have these settings in the build setup, as that means it can be managed centrally and doesn't have to be repeated in all index.adoc for all Spring Data projects. Is there a reason to add them here?

Jay Bryant added 4 commits April 12, 2018 10:12
I edited for spelling, punctuation, grammar, and corporate voice. I also added cross-references in a few places.
I added Tocbot and the files it requires (the tocbot-3.0.2 directory, docinfo.html, and docinfo-footer.html). I also moved spring.css into the stylesheets directory, to mirror our other projects. I also moved Asciidoctor attributes into index.adoc, so that command-line generation works correctly. Finally, I caught a couple of headings that weren't properly capitalized.
Mark proofread my first commit and caught a few things. I have fixed them. Thanks, Mark. :)
Mark Paluch and Oliver Gierke want to split the PR into a content portion and a ToC modernizing portion, so that we can do the ToC modernizing differently. To that end, I am removing all the ToC features from the PR.
I had switched to a different location for the stylesheet (to make this project look like the other Spring projects), but I then removed the stylesheet pursuant to changes requested by Oliver Gierke. That left the reference guide with no stylesheet. So I am putting back the former stylesheet arrangement.
mp911de pushed a commit that referenced this pull request Apr 13, 2018
Edited for spelling, punctuation, grammar, and corporate voice. Also added cross-references in a few places. Original pull request: #281.
mp911de added a commit that referenced this pull request Apr 13, 2018
Typo fixes, align Enable…Repositories annotation wording, document reactive return types. Original pull request: #281.
mp911de pushed a commit that referenced this pull request Apr 13, 2018
Edited for spelling, punctuation, grammar, and corporate voice. Also added cross-references in a few places. Original pull request: #281.
mp911de added a commit that referenced this pull request Apr 13, 2018
Typo fixes, align Enable…Repositories annotation wording, document reactive return types. Original pull request: #281.
mp911de pushed a commit that referenced this pull request Apr 13, 2018
Edited for spelling, punctuation, grammar, and corporate voice. Also added cross-references in a few places. Original pull request: #281.
mp911de added a commit that referenced this pull request Apr 13, 2018
Typo fixes, align Enable…Repositories annotation wording, document reactive return types. Original pull request: #281.
@mp911de
Copy link
Member

mp911de commented Apr 13, 2018

That's merged, polished, and backported now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants