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

Introduce MapIsEmpty Refaster rule #339

Merged
merged 4 commits into from
Nov 9, 2022
Merged

Introduce MapIsEmpty Refaster rule #339

merged 4 commits into from
Nov 9, 2022

Conversation

mlrprananta
Copy link
Contributor

Adds an additional refaster Map rule that builts on #337

Suggested commit message:

Introduce `map.keySet().isEmpty()` to `map.isEmpty()` refaster rule

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.

Nice! Will do a more thorough review later :)

@rickie rickie added this to the 0.6.0 milestone Nov 8, 2022
@rickie rickie force-pushed the mlrprananta/PSM-1547 branch from b6732fa to ff9415d Compare November 8, 2022 09:06
@rickie rickie force-pushed the sschroevers/refaster-map-improvements branch from 0b38be5 to 02397eb Compare November 8, 2022 09:14
@rickie rickie force-pushed the mlrprananta/PSM-1547 branch from ff9415d to 0647b3e Compare November 8, 2022 09:21
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.

Added a commit.

Suggested commit message:

Prefer `Map#isEmpty()` over `Map#{keySet,values,entrySet}.isEmpty()` (#339)

(Not sure how to chain method calls with the #...)

Alternative would be:

Introduce `MapIsEmpty` Refaster rule (#339)

One last thing to consider, we could sort the keySet, values and entrySet lexicographically but I think this order also makes sense 🤔.

@@ -44,4 +44,15 @@ Stream<String> testMapKeyStream() {
Stream<Integer> testMapValueStream() {
return ImmutableMap.of("foo", 1).entrySet().stream().map(Map.Entry::getValue);
}

Stream<boolean> testMapIsEmpty() {
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't flagged because of a tiny line of code missing in the base PR 😬. Will drop this method :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, overlooked that

@mlrprananta
Copy link
Contributor Author

Yeah I was thinking of ordering it lexicographically, but I wanted to stick with the order Stephan already proposed in the other rule

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.

W.r.t. ordering: it seems we often order by some fuzzy "frequency" metric. 🤷

Added one commit; don't ask me to exactly explain this ordering; also just following some existing fuzzy logic 😬. (Eventually we'll auto-sort this stuff.)

I think Introduce `MapIsEmpty` Refaster rule (#339) matches what we've done before 👍

Base automatically changed from sschroevers/refaster-map-improvements to master November 9, 2022 07:54
@mlrprananta
Copy link
Contributor Author

Rebased

@rickie rickie changed the title Introduce rule to rewrite map.keySet().isEmpty() to map.isEmpty() Introduce MapIsEmpty Refaster rule Nov 9, 2022
@rickie rickie merged commit b6146ef into master Nov 9, 2022
@rickie rickie deleted the mlrprananta/PSM-1547 branch November 9, 2022 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants