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

Relax / revert @stylistic/comma-dangle to ease migration from standard #79

Closed
voxpelli opened this issue Jun 26, 2024 · 10 comments · Fixed by #139
Closed

Relax / revert @stylistic/comma-dangle to ease migration from standard #79

voxpelli opened this issue Jun 26, 2024 · 10 comments · Fixed by #139
Assignees
@voxpelli
Milestone
1.0.0

Comments

@voxpelli
Copy link
Member

As both #78 (comment) and fastify/fastify#5509 mentioned, the enforcing of comma-dangle (that is one of two style related modernizations since standard@17.x) is a bit too prescriptive and maybe even controversial.

Related rule:

neostandard/lib/configs/modernization.js

Lines 23 to 29 in 3130ca9

'@stylistic/comma-dangle': ['warn', {
arrays: 'always-multiline',
objects: 'always-multiline',
imports: 'always-multiline',
exports: 'always-multiline',
functions: 'never',
}],

As mentioned in fastify/fastify#5509 (comment) the equivalent in standard@17.x was:

'@stylistic/comma-dangle': ['error', {
  arrays: 'never',
  objects: 'never',
  imports: 'never',
  exports: 'never',
  functions: 'never',
}],

#78 (comment) suggests adding a configuration for the "legacy" behavior, something like noCommaDangle: true or legacyCommaDangle: true, but I don't like adding too many options.

Another option would be to simply disable the rule completely and conclude that there is no consistent approach to this within the community anymore.

A third is to revert it completely, but that would make projects that then override it not be neostandard compliant anymore.

I think its safe to say that the current state of it violates the mission statement (#7) of Being a descriptive (not prescriptive) linting of common and good practices
This was referenced Jun 26, 2024
Closed
Merged
@jsumners
Copy link

Some thoughts:

  1. Enforcing this by default results in many, some would say irrelevant, changes when applied to code base already using standard
  2. I understand why people like it, but I really don't think it adds enough utility to overcome the aesthetics
  3. I agree that adding options is a path toward "what's the point?" There should be as few options as possible

@voxpelli
Copy link
Member Author

@jsumners Would you vote for removing the rule or to reverting to forbidding dangling commas?

@jsumners
Copy link

I think I would vote for removing the rule under the assumption that it would be the most permissive.

@bcomnes
Copy link
Member

bcomnes commented Jun 26, 2024

Can we leave it as a default-off option similar to semi rules?

@jerome-benoit
Copy link
Contributor

jerome-benoit commented Jun 27, 2024 •
edited
Loading

People migrating to neostandard are warned about that style change from standard. And as such have implicilty agreed upon.

But I do not see why neostandard could not offer:

  • modernisation -> neostandard rules
  • standard -> legacy standard rules

On the implementation side, I do not think making a literal with boolean options to add is a good idea. It will make the life of users and maintainers better if:

  • a style (or rules or configBase or ...) option is added
  • style can be:
    • modernisation -> neostandard rules (default)
    • standard -> standard rules

The existing boolean seems to be orthogonal enough to the base rules configuration to stay as is. But that can also be folded into the style namespace, such as semimodernisation.

This was referenced Jul 2, 2024
Merged
Open
@voxpelli voxpelli added this to the 1.0.0 milestone Jul 5, 2024
@voxpelli voxpelli mentioned this issue Aug 19, 2024
Merged
@voxpelli
Copy link
Member Author

undici is another project where this caused an issue: nodejs/undici#3485

@voxpelli voxpelli self-assigned this Aug 19, 2024
voxpelli added a commit that referenced this issue Aug 19, 2024
@voxpelli
feat: partly disable @stylistic/comma-dangle
890b0b4 
Fixes #79

Since there is no consensus on whether comma-dangle should be forbidden or required we should simply just ignore them for arrays, objects, imports, exports

All non-ignored defaults to "never" – such as `functions`, `enums`, `generics`, `tuples` – so no need to add these
@voxpelli voxpelli mentioned this issue Aug 19, 2024
Merged
@voxpelli
Copy link
Member Author

@jsumners @Uzlopak Added a PR that disables the contentious ones: #139

@Uzlopak
Copy link

Uzlopak commented Aug 19, 2024

I personally prefer the dangling comma, but projects like undici would be about adding 3211 missing dangling commas. I dont think that this is nice for existing projects, basically pointing the blame to the one who migrates :-P

@voxpelli voxpelli mentioned this issue Aug 20, 2024
Open
@voxpelli
Copy link
Member Author

@Uzlopak Lets find a way to reintroduce them in a less intrusive way: #143

@jsumners
Copy link

I personally prefer the dangling comma, but projects like undici would be about adding 3211 missing dangling commas. I dont think that this is nice for existing projects, basically pointing the blame to the one who migrates :-P

Given that eslint will add it on document save, this is the only real argument I can make against it. Adding it to projects that have not been using it results in way too many changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@voxpelli voxpelli

Labels
None yet
Projects
None yet
Milestone
1.0.0
Development

Successfully merging a pull request may close this issue.

feat: partly disable @stylistic/comma-dangle neostandard/neostandard
5 participants
@voxpelli @bcomnes @jsumners @Uzlopak @jerome-benoit