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

check trailing commas in list/set/map literals #3340

Merged
merged 1 commit into from
May 24, 2023
Merged

check trailing commas in list/set/map literals #3340

merged 1 commit into from
May 24, 2023

Conversation

a14n
Copy link
Contributor

@a14n a14n commented Apr 8, 2022

Description

This PR checks trailing commas in list/set/map literals.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 95.567% when pulling e476045 on a14n:improve-require_trailing_commas into 252bc11 on dart-lang:master.

bwilkerson
bwilkerson reviewed Apr 8, 2022
View reviewed changes
Copy link
Member

@bwilkerson bwilkerson left a comment

Choose a reason for hiding this comment

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

The implementation looks good.

I can't speak to whether the change in semantics is what users want, but this is likely to be a breaking change for at least some users.

test_data/rules/require_trailing_commas.dart Show resolved Hide resolved
@a14n
Copy link
Contributor Author

a14n commented Apr 8, 2022

but this is likely to be a breaking change for at least some users.

I agree but I don't think the creation of a new rule require_trailing_commas_for_list_set_map is worth it. This addition of list/set/maps looks consistant to the initial intent of this lint imho.

@pq
Copy link
Member

pq commented Apr 8, 2022

/cc @goderbauer

@goderbauer
Copy link
Contributor

Overall this looks reasonable to me. Have you by any chance already applied this to the Flutter code base to see if there are any surprising cases on a larger code base like that?

@a14n
Copy link
Contributor Author

a14n commented Apr 8, 2022

Correct me if I'm wrong but require_trailing_commas is not enabled on Flutter code base.

I will take a look among the 12K+ problems reported by VS code when I enable the current version of the lint.

@goderbauer
Copy link
Contributor

Correct me if I'm wrong but require_trailing_commas is not enabled on Flutter code base.

You are right, we never did enable it it looks like.

@a14n a14n mentioned this pull request Apr 26, 2022
Merged
@a14n
Copy link
Contributor Author

a14n commented Apr 26, 2022

I tested this PR on flutter and there are 700+ diagnostics about missing trailing commas in set/list/map literals. You can take a look at flutter/flutter#102585

What should we do with the current PR?

@goderbauer
Copy link
Contributor

I scrolled through flutter/flutter#102585. There are no surprises in there for me. So extending the trailing comma to collections still sounds reasonable to me.

@pq
Copy link
Member

pq commented Apr 27, 2022

My biggest concern on this is the time to migrate internal code that will break when we try and roll with this landed.

I'm guessing that the AddTrailingComma correction producer should get updated in server so we can do cleanup with dart fix?

If anyone wants to pitch in there, tests live here:

https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/test/src/services/correction/fix/add_trailing_comma_test.dart

@a14n a14n mentioned this pull request May 6, 2022
Closed
@a14n
Copy link
Contributor Author

a14n commented May 6, 2022

AddTrailingComma correction updated in dart-lang/sdk#48968 to support this PR

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request May 6, 2022
@a14n
add trailing comma fix for literals
04bcc30 
This PR extends the`add_trailing_comma` fix to handle for the [upcoming support of list/set/map literals in `require_trailing_commas`](dart-lang/linter#3340).

It's likely the tests in this PR will fail until sdk update the linter dependency to include dart-lang/linter#3340

Closes #48968

GitOrigin-RevId: 24b374e
Change-Id: I066a8524d18a4213eb5300b9310bdb4a562f0e27
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/243920
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@a14n
Copy link
Contributor Author

a14n commented May 6, 2022

AddTrailingComma correction updated in dart-lang/sdk#48968 to support this PR

@pq : There is now a fix to help the migration.

@pq
Copy link
Member

pq commented May 6, 2022

This is another one that will likely require a bunch of internal cleanup before landing in the SDK. I'm a bit oversubscribed at the moment but will put the feelers out for volunteers... Thanks @a14n!

@pq pq added the contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) label May 6, 2022
@a14n
Copy link
Contributor Author

a14n commented Jul 29, 2022

This is another one that will likely require a bunch of internal cleanup before landing in the SDK.

@pq is there a lot of internal cleanup to do?

@a14n a14n mentioned this pull request Sep 2, 2022
Closed
5 tasks
@srawlins
Copy link
Member

This rule is enabled internally, so I'll kick off a job to test it out.

@srawlins srawlins added the set-internal-available Affects a rule that is allowed internally (but not generally required or recommended). label Apr 17, 2023
@srawlins
Copy link
Member

There is much to clean up internally, haha. I've started the process. May take a few days. Thanks a million for your patience!!

@srawlins
Copy link
Member

Update, I have an in-progress change to fix the internal code but there are a lot of hoops to jump though, so I'm going to pause that. No ETA for when this can land.

@srawlins
Copy link
Member

@a14n this is now making great progress internally, would you mind making one more pass over the flutter repos before I land? I'm very excited for this one 😁

@a14n
Copy link
Contributor Author

a14n commented May 15, 2023

AFAICT this lint rule is disable in flutter/flutter.

@srawlins
Copy link
Member

Oh I see, woohoo! Thanks!

@pq any concerns with me landing then? Tracking for internal compliance is b/279356486.

@srawlins srawlins merged commit 25e4fd7 into dart-lang:main May 24, 2023
@a14n a14n deleted the improve-require_trailing_commas branch May 24, 2023 06:31
@srawlins
Copy link
Member

Quick note that this does not cover ListPatterns or MapPatterns. Mentioned in an open issue about records.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 23, 2023
@a14n
check trailing commas in list/set/map literals (dart-lang/linter#3340)
d87a566
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srawlins srawlins srawlins approved these changes

@bwilkerson bwilkerson bwilkerson approved these changes

Assignees
No one assigned
Labels
contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) set-internal-available Affects a rule that is allowed internally (but not generally required or recommended).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@a14n @coveralls @pq @goderbauer @srawlins @bwilkerson