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

Resolves #302: Bump PMD version to 6.10.0 #321

Merged
merged 1 commit into from Jan 17, 2019

Conversation

alecgrieser
Copy link
Contributor

@alecgrieser alecgrieser commented Jan 16, 2019

This updates the ruleset with their new file locations and tweaks the exclusions so that it mostly matches our coding style/level of cleanliness. It also makes some changes to match a few rules that I thought should be updated.

A few highlights:

  • I kept a rule stating that field declarations should come before anything else in a class. We were already (mostly) following that rule, with a few one-off exceptions of some constants (that I fixed--I don't think that should be controversial) as well as a few other cases where there were static methods or classes were defined before fields. In order to keep constructors near the fields they initialized, I decided to move some code around. The code in question generally had very little history (as it has been unchanged since the initial repository commit).
  • There is a rule called "PrematureDeclaration" that complains if one creates a local variable before a potential exit point. I began to fix those, but then I got to more complicated examples where it was unclear to me if rearranging lines was safe, so I just decided to disable the rule.

Before merging, there should probably be one more rebase from master (as this is prone to merge skew), and once it's merged, all other PRs should probably rebase from it for similar reasons.

This updates the ruleset with their new file locations and tweaks the exclusions so that it mostly matches are coding style/level of cleanliness. It also makes some changes to match a few rules that I thought should be updated.
@alecgrieser alecgrieser requested a review from MMcM January 16, 2019 23:46
public boolean isEquals() {
return low == high && low != null &&
return low != null && TupleHelpers.equals(low, high) &&
lowEndpoint == EndpointType.RANGE_INCLUSIVE && highEndpoint == EndpointType.RANGE_INCLUSIVE;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we could move these cheaper test earlier, but maybe the new TupleHelpers.equals is fast enough.

@@ -74,6 +74,7 @@
private RecordCursorResult<V> nextResult = RecordCursorResult.withNextValue(null, RecordCursorStartContinuation.START);

@SpotBugsSuppressWarnings("EI_EXPOSE_REP2")
@SuppressWarnings("PMD.UnusedFormalParameter") // for compatibility reasons
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it may actually be a bug, but this is okay until that gets settled.

@MMcM
Copy link
Contributor

MMcM commented Jan 17, 2019

one more run before merging to make sure no skew

test this please

@MMcM MMcM merged commit 583b92e into FoundationDB:master Jan 17, 2019
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.

None yet

2 participants