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 rules metadata to include if it's deprecated or not #2622

Closed
alexilyaev opened this issue Jun 12, 2017 · 21 comments · Fixed by #2871
Closed

Add rules metadata to include if it's deprecated or not #2622

alexilyaev opened this issue Jun 12, 2017 · 21 comments · Fixed by #2871
Labels
status: wip is being worked on by someone type: documentation an improvement to the documentation

Comments

@alexilyaev
Copy link
Contributor

alexilyaev commented Jun 12, 2017

Hi guys, I'm working on stylelint-find-rules.
I found myself wondering if I've set all rules that Stylelint offers or not, so I"ve created a package that lists rules available by Stylelint but not set in the user config.

This is similar to eslint-find-rules.

One thing that pops out is the deprecated rules (in Stylelint itself).
Usually you don't care about them when you're exploring new options.

There's already a thread about it in find-eslint-rules:
sarbbottam/eslint-find-rules#172

I suppose the easiest thing would be to add rule.deprecated = true; for each rule that's deprecated.

Right now I"m parsing the README.md file for each rule and searching for deprecated with RegEx 😀.

Can we add such a property?

@alexander-akait
Copy link
Member

/cc @stylelint/core

@alexander-akait alexander-akait added the status: needs discussion triage needs further discussion label Jun 12, 2017
@jeddy3
Copy link
Member

jeddy3 commented Jun 12, 2017

I suppose the easiest thing would be to add rule.deprecated = true; for each rule that's deprecated.

SGTM.

@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 Jun 12, 2017
@hudochenkov
Copy link
Member

I don't think it's a good idea. Of course, it's a small thing, and it's easy to add. But it's for one plugin only. And it's one more thing to keep in mind. We have a lot of things to keep in mind already.

Cool plugin, though.

@alexander-akait
Copy link
Member

alexander-akait commented Jun 12, 2017

@hudochenkov this util is very important for who have own rules, it is allow to caught rules which your missed, my opinion is it very usefully feature.

@hudochenkov
Copy link
Member

@evilebottnawi no doubts. But seems like @alexilyaev found a workaround, and no one else needs this functionality.

@alexilyaev
Copy link
Contributor Author

@hudochenkov Logic says that most things ESLint encounters in terms of ecosystem, Stylelint will encounter in some way or another.

So ESLint exposes rules as an Object with meta (which has deprecated, see example), and create function. They even added an API for getting all rules eslint/eslint#6525.

This is very flexible and opens the door for different kinds of tools to be built on it.

Note that right now I"m parsing a chunk of README files and searching for "deprecated". That's not a good solution at all 😀
Not fast, not reliable, etc.

Reminding again about sarbbottam/eslint-find-rules#172, it's a similar discussion. The use case for us would be to check if there are rules available that we haven't configured (or deprecated rules that need to be updated) so we could integrate it in CI and such.

@alexander-akait
Copy link
Member

@alexilyaev can your do PR? This is be great!

@jeddy3
Copy link
Member

jeddy3 commented Jun 13, 2017

This is very flexible and opens the door for different kinds of tools to be built on it.

Historically we've always sided in favour of adding means to extend/build-on stylelint (plugins, custom syntaxes, processors etc...). I think exporting some meta data is in tune with that.

So ESLint exposes rules as an Object with meta (which has deprecated, see example)

This SGTM.

@alexilyaev
Copy link
Contributor Author

@evilebottnawi Updating all rules to export an Object instead of a Function would be pretty big change :-). Also not familiar with the project code, not sure what it affects.

If you'd guys agree on the change and direct me to what files would be affected by changing all rules export, I could do it.
Guess it would require a codemod 🤖

@jeddy3
Copy link
Member

jeddy3 commented Jun 13, 2017

The stylelint rules are exposed as part of the public API.

Would simply adding a meta object onto each of the deprecated rules suffice? e.g.

For block-no-single-line:

// lib/rules/block-no-single-line/index.js#L61
rule.meta = { deprecated: true }
// stylelint-find-rules
const stylelint = require('stylelint')
console.log(stylelint.rules['block-no-single-line'].meta)

@alexilyaev
Copy link
Contributor Author

@jeddy3 I see.
In that case changing the export to an Object will break all plugins since they depend in the export being a function.

Adding stuff to the function properties feels weird, but then again, a function is an object, so...

Your suggestion is easy to add and won't break anything, so I'll go with it.

Any place we should add documentation for it?

@jeddy3
Copy link
Member

jeddy3 commented Jun 14, 2017

Having said all that, I suggest we hold off until after 8.0.0 is out soon. It's going to remove all the deprecated rules. There's a fair bit of reconciling of the master and v8 branches to do, and minimising conflicts is going to make that process easier.

If we deprecate any rules after 8.0.0 we can add meta data to them.

@jeddy3 jeddy3 added this to the 8.0.x milestone Jun 14, 2017
@alexilyaev
Copy link
Contributor Author

@jeddy3 Sounds good.

U can close this if u want, or keep open if it helps.

@jeddy3
Copy link
Member

jeddy3 commented Jun 26, 2017

I'll close for now.

@jeddy3 jeddy3 closed this as completed Jun 26, 2017
@alexilyaev
Copy link
Contributor Author

@jeddy3 I'd like to ask to reconsider adding some meta-data, at least for deprecated.

Reason

I just had a false-positive in stylelint-find-rules, the tool said max-nesting-depth is deprecated.

So, it works by looking up the word "deprecated" in the README file of each rule, and turns out the README for max-nesting-depth has that word, but it's not deprecated:
https://github.com/stylelint/stylelint/blob/master/lib/rules/max-nesting-depth/README.md

To be precise, it was looking at the first 1024 characters of the README.
I'll lower that number to fix the issue, but it's still a hack, and I have no other way telling if a rule is deprecated.

I assume for most major versions we'll have deprecated rules, and it's useful to know which at a glance, that's why I built stylelint-find-rules in the first place.

@ntwb
Copy link
Member

ntwb commented Aug 11, 2017

Reopening now that stylelint 8.0.0 is out

As outlined above in #2622 (comment) sounds like a good approach, currently there are no rules pending to be deprecated though.

For now probably documenting how to deprecate a rule in docs/developer-guide/rules.md seems like a good start, #2285 is a recent PR that might be helpful in documenting the process

@ntwb ntwb reopened this Aug 11, 2017
@jeddy3
Copy link
Member

jeddy3 commented Aug 21, 2017

SGTM.

@alexilyaev You can assume any rules deprecated in the future will come with rule.meta = { deprecated: true }.

@jeddy3 jeddy3 added status: wip is being worked on by someone type: documentation an improvement to the documentation and removed status: ready to implement is ready to be worked on by someone type: enhancement a new feature that isn't related to rules labels Sep 10, 2017
@Donov4n
Copy link
Member

Donov4n commented Sep 4, 2020

Hi @jeddy3,

You can assume any rules deprecated in the future will come with rule.meta = { deprecated: true }.

Is this still valid? Because the recent deprecated rules (*-(blacklist|whitelist)) don't seem to have this meta :/
if it's an omission, I can see to add it.

@jeddy3
Copy link
Member

jeddy3 commented Sep 13, 2020

Is this still valid?

Yes, it was a mistake not to include it.

if it's an omission, I can see to add it.

Yes, please. Can you also add an item to this list in the developer guide so that we don't forget to do this again in the future, thanks!

@Donov4n
Copy link
Member

Donov4n commented Sep 15, 2020

Can you also add an item to this list in the developer guide so that we don't forget to do this again in the future, thanks!

In fact, this seems to be already the case: "2. Add the appropriate meta data to mark the rule as deprecated.".
This line was added by you for this exact purposes (see the PR #2871 above)

--

All right, i will create a PR as soon as possible with the meta added to the deprecated rules.

@jeddy3
Copy link
Member

jeddy3 commented Sep 16, 2020

This line was added by you for this exact purposes (see the PR #2871 above)

😅

All right, i will create a PR as soon as possible with the meta added to the deprecated rules.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone type: documentation an improvement to the documentation
Development

Successfully merging a pull request may close this issue.

6 participants