add nullability annotations#1494
Conversation
gavinking
requested review from
njr-11 and
otaviojava
and removed request for
otaviojava
njr-11
left a comment
njr-11
left a comment
There was a problem hiding this comment.
The changes will Nonnull and Nullable look fine. I noted a couple of minor things with some other changes in the PR.
Given how many places this modifies and the merge conflicts it is likely to cause (as well as soft merge conflicts), we should consider delaying merging it until after the current backlog of PRs is merged
| * <li>a {@link Query} or {@link jakarta.persistence.query.JakartaQuery} | ||
| * annotation specifying a query with an {@code ORDER BY} clause, nor</li> | ||
| * <li>a {@code jakarta.persistence.query.NativeQuery} annotation.</li> | ||
| * <li>a {@link jakarta.persistence.query.NativeQuery} annotation.</li> |
There was a problem hiding this comment.
We definitely want to change this to link, but I noticed that afterward the build generates the Javadoc without any link and without any package, as,
which is worse than just {@code which at least includes the package name when there is no link. Are there more build steps that are needed to get it to find Persistence 4.0?
There was a problem hiding this comment.
@njr-11 Fixed now, I believe, but please test it.
I strongly suspect that the merge conflicts will be pretty trivial, but also FYI Codex is fantastic at resolving merge conflicts, even highly nontrivial ones. |
njr-11
left a comment
njr-11
left a comment
There was a problem hiding this comment.
Looks good now
I have no idea what we could have set up differently that would cause these inconsistencies.
I don't know why we are getting inconsistent warnings either. Probably the best thing to do in the hopefully rare cases where this occurs will be to align with the maven build output and then put a comment in the code where the SuppressWarnings would be indicating that the warning suppression should either be left there or not added, so that we don't go back and forth on adding and removing the suppression. In this case, it looks like Maven is okay with removing SuppressWarnings, so we can remove it and add the note so that I don't put it back in later when I see it.
the merge conflicts it is likely to cause
I strongly suspect that the merge conflicts will be pretty trivial, but also FYI Codex is fantastic at resolving merge conflicts, even highly nontrivial ones.
Every outstanding PR that adds a method will have a soft merge conflict with this. Nothing will show up as a merge conflict, but those PRs will be lacking the Nonnull/Nullable
| * <li>a {@link Query} or {@link jakarta.persistence.query.JakartaQuery} | ||
| * annotation specifying a query with an {@code ORDER BY} clause, nor</li> | ||
| * <li>a {@code jakarta.persistence.query.NativeQuery} annotation.</li> | ||
| * <li>a {@link jakarta.persistence.query.NativeQuery} annotation.</li> |
Sure, but nothing a robotic review won't quickly catch. |
| if ( expression == null ) { | ||
| throw new IllegalStateException( | ||
| Messages.get("013.no-expression")); | ||
| } |
|
I have updated to resolve the conflicts. |
2 participants
These were mostly robotically generated. I have done some review, but perhaps not quite as thorough as I should.