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

Minimal RocksJava compliance with Java 8 language level (EB 1046) #10951

Closed
wants to merge 10 commits into from

Conversation

alanpaxton
Copy link
Contributor

@alanpaxton alanpaxton commented Nov 14, 2022

Apply a small (and automatic) set of IntelliJ Java inspections/repairs to the Java interface to RocksDB Java and its tests.
Partly enabled by the fact that we now (from RocksDB7) require java 8.

Explicit

in empty lines in javadoc comments.

Parameters and variables made final where possible.
Anonymous subclasses converted lambdas.

Some tests which previously used other assertion models were converted to assertj, e.g. (assertThat(actual).isEqualTo(expected)

In a very few cases tests were found to be inoperative or broken, and were repaired. No problems with actual RocksDB behaviour were observed.

This PR is intended to replace #9618 - that PR was not merged, and attempts to rebase it have yielded a questionable looking diff, so we choose to go back to square 1 here, and implement a conservative set of changes.

@alanpaxton alanpaxton marked this pull request as draft November 14, 2022 10:20
@alanpaxton alanpaxton force-pushed the eb-1046-java8-minimal-fix branch 2 times, most recently from 60aafa4 to ef5bfe7 Compare November 15, 2022 11:52
@alanpaxton alanpaxton marked this pull request as ready for review November 21, 2022 08:26
@alanpaxton
Copy link
Contributor Author

Hi @jay-zhuang you were good enough to review a previous PR in this area which I've had to abandon because I had missed that it hadn't got to the merge point; it changed a lot of files and I wasn't confident enough of the rebase, so I started again with reduced scope to get some improvements suggested by Java8 inspections incorporated. Thanks.

@adamretter adamretter self-requested a review November 22, 2022 10:57
@adamretter
Copy link
Collaborator

Hi @ajkr @siying, could you take a look at importing/merging this one please?

Copy link
Collaborator

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

LGTM

use java 8 flags in build
add final on params and variables
fix modifier order where wrong
<p> in comment blank lines
use enhanced for loop where applicable
 (

add final on params and variables
fix modifier order where wrong
<p> in comment blank lines
use enhanced for loop where applicable
try-with-resources, ignored variable names.
Method was removed after current branch commenced.
Somehow (badly managed rebase by me ?) it got added back.
Manually remove it again.
@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@alanpaxton has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ajkr merged this pull request in e110d71.

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

Successfully merging this pull request may close these issues.

None yet

5 participants