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 configurationComment configuration property #6629

Merged
merged 14 commits into from Mar 16, 2023

Conversation

ifitzpatrick
Copy link
Contributor

@ifitzpatrick ifitzpatrick commented Feb 8, 2023

Closes #6628

This PR changes the list of stylelint commands to omit the prefix, and updates functions related to stylelint commands to accept the prefix as an argument. Additionally it makes stylelintCommandPrefix configurable to the users needs.

@changeset-bot
Copy link

changeset-bot bot commented Feb 8, 2023

🦋 Changeset detected

Latest commit: e975776

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ifitzpatrick ifitzpatrick force-pushed the feat/configure-command-prefix branch 3 times, most recently from 5d4258b to f3cb94c Compare February 9, 2023 22:09
@jeddy3 jeddy3 changed the title Make stylelintCommand prefix configurable Add disablesPrefix configuration property Feb 10, 2023
@Mouvedia
Copy link
Contributor

This page will have to be updated.

@ifitzpatrick
Copy link
Contributor Author

Updated the docs and changed the name of the configuration property to disablesPrefix (#6628 (comment))

@ifitzpatrick ifitzpatrick force-pushed the feat/configure-command-prefix branch 3 times, most recently from 5cccf34 to d2ba0dc Compare February 11, 2023 23:36
@ybiquitous
Copy link
Member

Considering we have also stylelint-enable, is the disablesPrefix name suitable? Is commandPrefix (or something else) better?

@jeddy3
Copy link
Member

jeddy3 commented Mar 14, 2023

@ybiquitous See #6628 (comment) as disables is consistent with our relevant options. Although, it's not a strong preference.

@jeddy3
Copy link
Member

jeddy3 commented Mar 14, 2023

Is commandPrefix (or something else) better?

We may apply this to more than disables and enables in the future, e.g. configuring rules. So, on further thought, maybe there is a better name than disablesPrefix.

ESLint uses "configuration comments" to describe disabling comments too. Should we adopt that terminology for the option name (and internally so that things are consistent)? It's aligned with our language of configuring rules.

The option could be configurationComment, defaulting to "stylelint".

{
  "configurationComment": "foo-stylelint"
}

Internally, we'd change:

  • isStylelintCommand(comment) to isConfigurationComment(comment)
  • extractStylelintCommand(comment) to extractStylelintComment(comment)
  • and so on

We, rather than the user, should add a hyphen before any of our existing command, e.g.:

  • const DISABLE_COMMAND = 'disable'; becomes const DISABLE_COMMENT = '-disable';

How does that sound?

@ifitzpatrick
Copy link
Contributor Author

ifitzpatrick commented Mar 14, 2023

Is there any concern that changing the names of isStylelintCommand and friends would be a breaking change for rules or plugins outside of this repo?

@ybiquitous
Copy link
Member

@jeddy3's idea sounds good to me. 👍🏼

Is there any concern that changing the names of isStylelintCommand and friends would be a breaking change for rules or plugins outside of this repo?

I believe there is no concern because isStylelintCommand is an internal utility, and we have not supported it as a public API.

Just in case, I searched the usage of isStylelintCommand in GitHub, but there seem to be no plugins using the utility.

@ifitzpatrick
Copy link
Contributor Author

Awesome thanks for looking into that @ybiquitous! Updated those functions and filenames to configurationComment.

docs/user-guide/configure.md Outdated Show resolved Hide resolved
docs/user-guide/configure.md Outdated Show resolved Hide resolved
@ybiquitous ybiquitous changed the title Add disablesPrefix configuration property Add configurationComment configuration property Mar 15, 2023
@ybiquitous ybiquitous changed the title Add configurationComment configuration property Add configurationComment configuration property Mar 15, 2023
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes. It's shaping up nicely.

I've requested a couple of documentation changes and asked one questino.

.changeset/wet-yaks-sneeze.md Outdated Show resolved Hide resolved
docs/user-guide/configure.md Outdated Show resolved Hide resolved
docs/user-guide/configure.md Outdated Show resolved Hide resolved
docs/user-guide/configure.md Outdated Show resolved Hide resolved
lib/rules/comment-empty-line-before/index.js Outdated Show resolved Hide resolved
docs/user-guide/configure.md Outdated Show resolved Hide resolved
@jeddy3
Copy link
Member

jeddy3 commented Mar 15, 2023

I pushed 3 commits to standardise the terminology elsewhere in our docs as part of this PR.

Comment on lines 141 to +143
#### `"stylelint-commands"`

Ignore comments that deliver commands to stylelint, e.g. `/* stylelint-disable color-no-hex */`.
Ignore configuration comments, e.g. `/* stylelint-disable color-no-hex */`.
Copy link
Member

Choose a reason for hiding this comment

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

I hadn't realised we had an option related to commands. I think we're ok to move forward with "configuration comment", though, as we have a few other rule secondary options which are at odds with our conventions.

ifitzpatrick and others added 6 commits March 15, 2023 13:48
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM 👍🏼

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

@jeddy3 jeddy3 merged commit 4fadc6c into stylelint:main Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add disablesPrefix configuration property
4 participants