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

Document JedisClusterCommand.releaseConnection() as Nullable #2356

Closed
wants to merge 1 commit into from

Conversation

walles
Copy link
Contributor

@walles walles commented Jan 25, 2021

This helps both people (me!) and machines (IntelliJ) know how to call this method.

Also, for IntelliJ users, if you find the Constant conditions & exceptions inspection and enable Suggest @Nullable annotation ... where nullable values are used you will be informed about potential runtime NullPointerExceptions.

A lot of things could probably be annotated, but I wanted to start with something to encourage future PRs to add nullability annotations where appropriate.

This helps both people (me!) and machines (IntelliJ) know how to call
this method.

Also, for IntelliJ users, if you find the "Constant conditions &
exceptions" inspection and enable "Suggest @nullable annotation ...
where nullable values are used" you will be informed about potential
runtime NullPointerExceptions.

A lot of things could probably be annotated, but I wanted to start with
*something* to encourage future PRs to add nullability annotations where
appropriate.
@walles
Copy link
Contributor Author

walles commented Jan 25, 2021

Would have helped me in #2355.

@sazzad16
Copy link
Collaborator

@walles Include the change in your actual PR. But it is worth mentioning that it is unlikely to pass. We are trying to keep Jedis with least number of dependencies.

@sazzad16 sazzad16 closed this Jan 25, 2021
@walles
Copy link
Contributor Author

walles commented Jan 25, 2021

I would like this and #2355 to be reviewed independently and merged (or not) independently.

While having had this PR merged first would have saved me some time in #2355, they are functionally independent.

@sazzad16
Copy link
Collaborator

In that case, consider it rejected because of the new dependency.

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