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

Add --rule and --no-stylelintrc CLI flags #4331

Closed
shawnbot opened this issue Oct 7, 2019 · 26 comments
Closed

Add --rule and --no-stylelintrc CLI flags #4331

shawnbot opened this issue Oct 7, 2019 · 26 comments
Labels
status: needs discussion triage needs further discussion

Comments

@shawnbot
Copy link
Contributor

shawnbot commented Oct 7, 2019

What is the problem you're trying to solve?

Stylelint autofixes turn out to be a really good way to refactor large codebases. However, it's painful having to modify your config by hand, and if you're extending an external config it's even trickier.

What solution would you like to see?

My proposal is a rule filter that could be enabled via the CLI, as in:

stylelint --rule-filter=some-rule --fix

or via the Node API:

const stylelint = require('stylelint')
stylelint.lint({
  files: '**/*.scss',
  fix: true,
  ruleFilter: 'some-rule'
})

Ideally, rules would be matched against the fully loaded config. It could be as simple as:

// anymatch allows for the same glob patterns as the "files" option
const anymatch = require('anymatch')
const {rules} = finalConfig
const {ruleFilter} = options
if (ruleFilter) {
  const filterRuleName = anymatch(ruleFilter)
  for (const ruleName of Object.keys(rules)) {
    if (!filterRuleName(ruleName)) {
      delete rules[ruleName]
    }
  }
}

Prior art

eslint has a --rule option, as mentioned in eslint/eslint#6333. Generally I think parity between the two is good, so having a --rule option would be nice, too.

There's also a standalone tool called eslint-nibble that provides a nicer UX for tackling lots of rule violations. This is probably the route that I'll go if there isn't any interest in making these changes to stylelint directly. Either way, I'm happy to help with the implementation too.

@hudochenkov
Copy link
Member

I don't understand the usefulness of this feature.

However, it's painful having to modify your config by hand

Isn't it the same as modifying CLI command?

@hudochenkov hudochenkov added the status: needs discussion triage needs further discussion label Oct 12, 2019
@shawnbot
Copy link
Contributor Author

Isn't it the same as modifying CLI command?

What I'm proposing is that --rule would disable all the other rules so that you could focus on a single one. So if your config has dozens (if not hundreds) of rules, then the answer to your question is no.

One problem with doing it in a standalone tool is having to reimplement all of the same configuration loading logic that stylelint does. If that's just using the cosmiconfig API, then I guess it's not a huge deal. But I still think that giving people a way to focus on one or more rules — even just to spot-check a change they're making, particularly in big refactors — would be super useful.

@hudochenkov
Copy link
Member

hudochenkov commented Oct 27, 2019

It will be confusing for people who use --rule in ESLint. There --rule specify a rule.

One problem with doing it in a standalone tool is having to reimplement all of the same configuration loading logic that stylelint does.

We have --print-config flag, which could be used to get config for a file. It prints into terminal. We don't have Node.js API for that.

I see you're using it in unofficial way :)

@shawnbot
Copy link
Contributor Author

shawnbot commented Oct 30, 2019

It will be confusing for people who use --rule in ESLint. There --rule specify a rule.

Ah yeah, good point. 🤔 If you're still open to this idea, I'd be happy to hash out alternatives, such as:

  • --only, as in --only some-rule or --only some-rule,another-rule
  • --rules (plural) as in --rules some-rule,another-rule

My original proposal for --rule-filter is more explicit, though, so that might still be the best bet.

We have --print-config flag, which could be used to get config for a file. It prints into terminal. We don't have Node.js API for that.

I see you're using it in unofficial way :)

Yeah, it was the best way that I see to do it without spawning a stylelint --print-config CLI process and capturing stdout. 😬

@hudochenkov
Copy link
Member

I see this feature as not something that would benefit majority of our users. Therefore, I think stylelint-only is a good solution. As it provides extra functionality for people who need it.

I'm curious what @stylelint/contributors think.

@ntwb
Copy link
Member

ntwb commented Oct 30, 2019

Looking at the ESLInt --rule docs could stylelint do the same:

stylelint --rule 'quotes: [2, double]' --no-stylelintrc

That brings parity of ESLint to stylelint, also allows only a single rule to be used?

@nosilleg
Copy link
Contributor

stylelint-only looks very useful for the reasons you've stated here. I need to keep it in mind!

That being said I'm not sure it's core functionality for this project and my current opinion is leaning towards it remaining separate.

@BPScott
Copy link
Contributor

BPScott commented Oct 31, 2019

That brings parity of ESLint to stylelint, also allows only a single rule to be used?

Looks like you can specify --rule multiple times to build up multiple rules

eslint --rule 'guard-for-in: 2' --rule 'brace-style: [2, 1tbs]'

I'm all for API parity where possible to help people easily move between tools and I think this falls under that remit

@ntwb
Copy link
Member

ntwb commented Oct 31, 2019

Also note that without the --no-eslintrc flag any rules specified are merged into any existing configurations

@jeddy3
Copy link
Member

jeddy3 commented Jan 11, 2020

I'm curious what @stylelint/contributors think.

Funnily enough, I needed this feature earlier today when working through stylelint/stylelint-demo#27 (comment) as I would've liked to have done:

% echo "a:b(a:(f)) {}" | npx stylelint --rule 'selector-no-vendor-prefix: true' --no-stylelintrc

I'd be happy to see the same --rule behaviour as ESLint brought to stylelint. As @ntwb said:

Also note that without the --no-eslintrc flag any rules specified are merged into any existing configurations


Shall I mark as an enhancement and help wanted?

@jeddy3 jeddy3 changed the title Filter individual rules via the CLI Add --rule and --nostylelintrc CLI flags Jan 11, 2020
@jeddy3 jeddy3 changed the title Add --rule and --nostylelintrc CLI flags Add --rule and --no-stylelintrc CLI flags Jan 11, 2020
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: enhancement a new feature that isn't related to rules and removed status: needs discussion triage needs further discussion labels Sep 25, 2021
@Mouvedia
Copy link
Contributor

IMO ruleFilter should accept either array or string and correspondingly we would have --rule (single rule but reusable several times) and --rules which would accept a file (JSON array) or a string (stringified array).

@betaorbust
Copy link

For the usability side: I frequently use stylelint as a rule-to-prevent-it-in-the-future as well as a codemod to fix current issues. I'm working in a very large monorepo where I can't go in and run --fix on everything at once because it would hit too many rules across the board and touch more code than I could verify.
Having the ability to select only one rule to run would be hugely helpful in this case.

@jeddy3
Copy link
Member

jeddy3 commented Oct 4, 2022

Thanks for chiming in with your use case, @betaorbust. It makes sense.

If anyone has time to implement these new options (--rule and --no-stylelintrc), please consider contributing.

--rule:

  • by default, merges rule to the end of the configuration object
  • can be used multiple times

--no-stylelintrc:

  • don't use the .stylelintrc file, and therefore make --rule(s) standalone

@ybiquitous
Copy link
Member

I now reconsider this issue.

--no-stylelintrc

Users can specify not only .stylelintrc* but also stylelint.config.* files. Additionally, they can use the --config flag to specify any file. So I'm concerned that the new flag name --no-stylelintrc would not be very intuitive.

Instead, --no-config seems more intuitive to me. What do you think?

FYI. ESLint has started using the new config file eslint.config.js recently.

--rule

The ESLint's --rule flag adopts the levn format. This format is more human-readable than JSON, but we need to add a new dependency on the levn library. Is it acceptable? Also, what do you think about the learning cost of users for the format?

For another point, if using --rule for a rule that long options like declaration-property-value-allowed-list, would it not difficult?

@ybiquitous
Copy link
Member

ybiquitous commented Nov 15, 2022

Reconsidering this issue's original motivation, it seems that the following option would be just required:

# Run with only rules defined in an existing config file (e.g. .stylelintrc.js)
stylelint --rule declaration-property-value-allowed-list "src/**/*.css"

# Or --only may be more intuitive than --rule
stylelint --only declaration-property-value-allowed-list "src/**/*.css"
// .stylelintrc.js
module.exports = {
  rules: {
    // other rules ...

    "declaration-property-value-allowed-list": {
      "transform": ["/scale/"],
      "whitespace": "nowrap",
      "/color/": ["/^green/"],
      // other values ...
    },

    // other rules ...
  }
};

@Mouvedia
Copy link
Contributor

Mouvedia commented Nov 15, 2022

--no-config

👍 (or --no-cfg)

--only

👎 (see #4331 (comment))

@Mouvedia Mouvedia self-assigned this Oct 19, 2023
@Mouvedia Mouvedia added status: wip is being worked on by someone and removed status: ready to implement is ready to be worked on by someone labels Oct 19, 2023
@jeddy3
Copy link
Member

jeddy3 commented Dec 19, 2023

We should have switched this issue back to needs: discussion because there are unanswered concerns and questions.

It feels like a 3rd party stylelint-nibble package is more appropriate, as there are only a couple of hearts/thumb-ups in the four years that the issue has been open, and we're moving towards simplifying our configuration model rather than adding more means to augment it.

@Mouvedia It's fantastic that you're eager to contribute and move Stylelint forward. But, as with all these historical issues, it's best to ask if they're still relevant before picking them up as many are still open (and labelled with ready to implement) simply because we've not had time to revisit them. For example, the issue predates overrides.

@jeddy3 jeddy3 added status: needs discussion triage needs further discussion and removed status: wip is being worked on by someone type: enhancement a new feature that isn't related to rules labels Dec 19, 2023
@Mouvedia
Copy link
Contributor

Mouvedia commented Dec 19, 2023

@Mouvedia It's fantastic that you're eager to contribute and move Stylelint forward. But, as with all these historical issues, it's best to ask if they're still relevant before picking them up as many are still open (and labelled with ready to implement) simply because we've not had time to revisit them.

It's the second time that I have picked up an issue which status has been reverted from ready to implement to needs discussion. Minutes before I was about to turn the draft to ready to review I saw your message.
I don't know what to think.

See also #7252 (comment) which prompted me to restart my efforts.

@ybiquitous
Copy link
Member

@Mouvedia Sorry that my comment confused you.

As @jeddy3 suggested, let's stop this now. We've recently started considering a new configuration system and CLI defaults (#7408 and #7409). The situation has changed.

@Mouvedia
Copy link
Contributor

Mouvedia commented Dec 20, 2023

The sudden reversal as I send the PR is kind of weird and not motivating me nor reassuring me.
The label system needs to be seriously reconsidered if you don't want to hinder contributions.
From now on I will ask explicitly on each issue without ever trusting the labels.


I think the --rule flag is useful. It helps the effort of us being in parity with ESLint on features that were requested by the community for justified use cases.

because there are unanswered concerns and questions.

What are the unanswered concerns and questions?
The only ones that I didn't cover in my PR were:

  • value-less rule picking
    e.g. --rule color-no-invalid-hex
  • --rules accepting a file
  • --no-cfg

@ybiquitous
Copy link
Member

@Mouvedia Sorry for the sudden reversal.

But you still have not answered my question (#7252 (comment)) since I was surprised by the --merge-rules suddenly came. You should have confirmed that the long-standing issue was still active and decisions were unchanged before sending a PR.

@Mouvedia
Copy link
Contributor

…since I was surprised by the --merge-rules suddenly came.

I can reduce the scope of the PR if you want. But IMHO this feature needs to reach a minimum threshold to be really useful.
Namely:

  • being able to switch the strategy (merge or isolate)
  • being able to provide a list instead of repeating --rule dozen of times
  • handling conflicts (i.e. precedence/priority/overriding)
  • providing the same level of granularity for the node options

What I am unsure about is whether I really need levn or not. It adds complexity, lacks a declaration file and is either too loose or too strict.

You should have confirmed that the long-standing issue was still active and decisions were unchanged before sending a PR.

I was trusting the label. More than @jeddy3's reversal, it's his timing that really surprised me.

@ybiquitous
Copy link
Member

@Mouvedia I believe there is still room to discuss new CLI flags and syntax for configuring a rule before entering implementation details.

Also, if a 3rd-party package would do the same instead, we'd not need to increase code or dependency. We have to consider if the Stylelint core should provide this feature or if it'd be worth it.

@jeddy3
Copy link
Member

jeddy3 commented Dec 20, 2023

I intended to catch this before more work was done. Unfortunately, I was too late. The timing was coincidental.

As I mentioned in the --ext issue, we need to revisit our historical issues, but no one has found the time to do that yet. We could rename the status: ready to implement label to something like status: ask to implement as a quick buffer.

We can also create a new issue to look into other ways of improving how we manage issues and contributions, e.g. automatically relabelling issues when they reach a certain age.

@Mouvedia
Copy link
Contributor

Mouvedia commented Jan 7, 2024

We could rename the status: ready to implement label to something like status: ask to implement as a quick buffer.

We could do that and slightly emend the issue template.

@jeddy3 should I close this issue and its PR?

@jeddy3
Copy link
Member

jeddy3 commented Jan 7, 2024

Yes, I think so. A 3rd-party package would help us avoid increasing our code and dependencies.

I've renamed the label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs discussion triage needs further discussion
Development

No branches or pull requests

9 participants