Description

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

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

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.

/cc @goderbauer

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?

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.

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.

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 https://github.com/flutter/flutter/pull/102585

What should we do with the current PR?

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

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

AddTrailingComma correction updated in https://github.com/dart-lang/sdk/pull/48968 to support this PR

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

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

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!

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?

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

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

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.

@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 😁

AFAICT this lint rule is disable in flutter/flutter.

Oh I see, woohoo! Thanks!

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

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