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.

Sign up for GitHub

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

Jump to bottom

Extend TimeRules Refaster rule collection #979

Merged
merged 6 commits into from
Jan 31, 2024
Merged

Extend TimeRules Refaster rule collection #979

merged 6 commits into from
Jan 31, 2024

Conversation

Venorcis
Copy link
Contributor

@Venorcis Venorcis commented Jan 16, 2024 •
edited

Uses e.g. LocalDate#plusDays instead of more contrived alternatives.
Quite a big PR as sadly none of the involved classes share a common interface for the shorthand methods.

@Venorcis Venorcis changed the title Use plus### and minus### shorthands when possible for dates/times Use plus### and minus### shorthands when possible for dates/times Jan 16, 2024
@github-actions GitHub Actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@rickie rickie self-requested a review January 16, 2024 09:58
Stephan202
Stephan202 reviewed Jan 16, 2024
View reviewed changes
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

When you asked me offline whether we could do this better, I didn't expect there to be this many cases, haha. Perhaps an Error Prone check would make sense, but it might be nice to get this merged first. (There are also other Refaster rules that may benefit from generalization.)

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java Outdated
@@ -582,4 +582,980 @@ Period after() {
return Period.ZERO;
}
}

/** Prefer {@link LocalDate#plusDays} (and variants) over more contrived alternatives. */
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't help my self and applied it to all rules; will push 😅

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java Outdated
Comment on lines 588 to 599
@BeforeTemplate
LocalDate before(LocalDate localDate, int days) {
return Refaster.anyOf(
localDate.plus(days, ChronoUnit.DAYS), localDate.plus(Period.ofDays(days)));
}
Copy link
Member

Choose a reason for hiding this comment

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

By matching against int here (for compatibility with the second case) we're not matching the first case for long parameters (LocalDate.EPOCH.plus(1L, ChronoUnit.DAYS) isn't matched).

Let me see whether I can regex a fix for this 😅

@github-actions GitHub Actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

2 similar comments
@github-actions GitHub Actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@github-actions GitHub Actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@Stephan202
Copy link
Member

Added two commits.

Suggested commit message:

Extend `TimeRules` Refaster rule collection (#979)

By introducing `SomeDateType{Plus,Minus}SomeUnit` Refaster rules, that suggest
e.g. `LocalDate#plusDays(long)` over more contrived alternatives.

@Stephan202 Stephan202 added this to the 0.15.0 milestone Jan 16, 2024
@Venorcis
Copy link
Contributor Author

Added two commits.

Suggested commit message:

Thanks @Stephan202! 🦸🏻‍♂️

@github-actions GitHub Actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

1 similar comment
@github-actions GitHub Actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie
rickie approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

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

Used my brain to do a lot of pattern matching during the review 🧠 .

Nice work guys 😄.

@github-actions GitHub Actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jan 31, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@rickie rickie merged commit 07fe6df into master Jan 31, 2024
16 checks passed
@rickie rickie deleted the vkoeman/adding-durations branch January 31, 2024 07:52
@rickie
Copy link
Member

rickie commented Jan 31, 2024

Nice suggested commit message @Stephan202 !

@Stephan202 Stephan202 changed the title Use plus### and minus### shorthands when possible for dates/times Extend TimeRules Refaster rule collection Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stephan202 Stephan202 Stephan202 approved these changes

@rickie rickie rickie approved these changes

Assignees
No one assigned
Labels
new feature
Milestone
0.15.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Venorcis @Stephan202 @rickie