Skip to content

DATACMNS-1601 - Improving the JavaDoc of the CrudRepository. #412

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

Closed
wants to merge 3 commits into from

Conversation

schauder
Copy link
Contributor

Clarifying expectations and guarantees or lack thereof.

https://jira.spring.io/browse/DATACMNS-1601

schauder and others added 3 commits October 29, 2019 14:51
Clarifying expectations and guarantees or lack thereof.
Document throws on save(…). Align documentation for reactive repositories.
Copy link
Contributor Author

@schauder schauder left a comment

Choose a reason for hiding this comment

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

I found a lot of tiny stuff.

*/
Mono<T> findById(Publisher<ID> id);

/**
* Returns whether an entity with the id exists.
* Returns whether an entity with the given {@code id} exists.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
* Returns whether an entity with the given {@code id} exists.
* Returns whether an entity with the given {@literal id} exists.

@@ -108,20 +109,29 @@
Flux<T> findAll();

/**
* Returns all instances with the given IDs.
* Returns all instances of the type {@code T} with the given IDs.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
* Returns all instances of the type {@code T} with the given IDs.
* Returns all instances of the type {@literal T} with the given IDs.

*/
Flux<T> findAllById(Iterable<ID> ids);

/**
* Returns all instances of the type with the given IDs supplied by a {@link Publisher}.
* Returns all instances of the type {@code T} with the given IDs supplied by a {@link Publisher}.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
* Returns all instances of the type {@code T} with the given IDs supplied by a {@link Publisher}.
* Returns all instances of the type {@literal T} with the given IDs supplied by a {@link Publisher}.

*/
Maybe<T> findById(Single<ID> id);

/**
* Returns whether an entity with the given id exists.
* Returns whether an entity with the given {@code id} exists.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
* Returns whether an entity with the given {@code id} exists.
* Returns whether an entity with the given {@literal id} exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm stopping commenting on this, since it is easier to find and fix within the IDE.

*
* @param id must not be {@literal null}.
* @return {@literal true} if an entity with the given id exists, {@literal false} otherwise.
* @throws IllegalArgumentException if {@code id} is {@literal null}.
* @return {@link Single} emitting {@literal true} if an entity with the given id exists, {@literal false} otherwise.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok it now gets really nitpicky but shouldn't it be TRUE since it is the constant from the wrapper type, not the primitive?

If you agree there are more places like this.

* @return {@literal true} if an entity with the given id exists, {@literal false} otherwise.
* @throws IllegalArgumentException if {@code id} is {@literal null}.
* @return {@link Single} emitting {@literal true} if an entity with the given id exists, {@literal false} otherwise.
* @throws IllegalArgumentException in case the given {@code id} is {@literal null}.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
* @throws IllegalArgumentException in case the given {@code id} is {@literal null}.
* @throws IllegalArgumentException in case the given {@literal id} is {@literal null}.

* @return {@literal true} if an entity with the given id exists, {@literal false} otherwise.
* @throws IllegalArgumentException if {@code id} is {@literal null}
* @return {@link Single} emitting {@literal true} if an entity with the given id exists, {@literal false} otherwise.
* @throws IllegalArgumentException in case the given {@link Single id} is {@literal null}.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be either {@literal IllegalArgument} or {@link IllegalArgument}

Same in many other places.

*/
Flowable<T> findAll();

/**
* Returns all instances of the type with the given IDs.
* Returns all instances of the type {@code T} with the given IDs.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are mixing IDs and ids and I think it should be {@literal ids} referring to the parameter name.

*/
Flowable<T> findAllById(Iterable<ID> ids);

/**
* Returns all instances of the type with the given IDs.
* Returns all instances of the type {@code T} with the given IDs supplied by a {@link Flowable}.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

mp911de pushed a commit that referenced this pull request Oct 30, 2019
Clarifying expectations and guarantees or lack thereof.

Original pull request: #412.
mp911de added a commit that referenced this pull request Oct 30, 2019
Document throws on save(…). Align documentation for reactive repositories.

Original pull request: #412.
mp911de pushed a commit that referenced this pull request Oct 30, 2019
Clarifying expectations and guarantees or lack thereof.

Original pull request: #412.
mp911de added a commit that referenced this pull request Oct 30, 2019
Document throws on save(…). Align documentation for reactive repositories.

Original pull request: #412.
mp911de pushed a commit that referenced this pull request Oct 30, 2019
Clarifying expectations and guarantees or lack thereof.

Original pull request: #412.
mp911de added a commit that referenced this pull request Oct 30, 2019
Document throws on save(…). Align documentation for reactive repositories.

Original pull request: #412.
mp911de added a commit that referenced this pull request Oct 30, 2019
Document throws on save(…). Align documentation for reactive repositories.

Original pull request: #412.
mp911de added a commit that referenced this pull request Oct 30, 2019
Document throws on save(…). Align documentation for reactive repositories.

Original pull request: #412.
mp911de added a commit that referenced this pull request Oct 30, 2019
Document throws on save(…). Align documentation for reactive repositories.

Original pull request: #412.
@mp911de
Copy link
Member

mp911de commented Oct 30, 2019

Updated to consistent literal usage. 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants