Skip to content
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

Enable PMD at Jakarta Data API #816

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Enable PMD at Jakarta Data API #816

wants to merge 5 commits into from

Conversation

otaviojava
Copy link
Contributor

No description provided.

@otaviojava otaviojava requested a review from njr-11 August 8, 2024 08:13
Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

These are mostly trivial changes that would normally be fine. However, because we are working on 1.0.1 which should be TCK updates only, we should not be editing the source code or spec yet.

Even small changes like this come with risks. An example of that is the addition of Array.copyOf to PageRequestCursor(Object... key), which is used by API method Cursor.forKey(Object... key). Given that the user is supplying the parameters as varargs, it is extremely unlikely to see a non-varargs array passed in instead, but nonetheless a user could do that. And if a user does so, and then modifies it afterward but before otherwise using the PageRequest, the user could be relying on a behavior that we would break by making this change. To emphasize, this is extremely unlikely. Given how unlikely it is for someone to pass a non varargs array to our varargs API method, a good argument could be made that we ought not to go out of our way and incur a performance penalty (copying arrays) for something that a user is really only ever going to break if they are trying to. It might be better just to add a line to the JavaDoc of Cursor.forKey saying if you pass a non-varargs array in as the varargs parameter, don't modify it afterward.

@njr-11 njr-11 added this to the 1.1 milestone Aug 8, 2024
@otaviojava
Copy link
Contributor Author

@njr-11, once we merge, could we move this one forward?

@njr-11
Copy link
Contributor

njr-11 commented Oct 1, 2024

@njr-11, once we merge, could we move this one forward?

Yes, once we merge the updates to switch to v1.1, we can start making code updates again, although we still need to watch out to avoid making breaking changes.

@otaviojava
Copy link
Contributor Author

@njr-11, once we merge, could we move this one forward?

Yes, once we merge the updates to switch to v1.1, we can start making code updates again, although we still need to watch out to avoid making breaking changes.

I did not change any API or any breaking changes; this only includes PMD and checkstyle.

@njr-11
Copy link
Contributor

njr-11 commented Oct 1, 2024

I did not change any API or any breaking changes; this only includes PMD and checkstyle.

See my prior review comment . One of the updates is technically a breaking change in behavior, even though it is unlikely applications would use it that way.

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