Deprecate *-blacklist/*-requirelist/*-whitelist #4892
Conversation
jeddy3
changed the title
There was a problem hiding this comment.
Looks like nobody has gotten around for the second review, but this looks generally good to me! On inspection and local testing, no problems 😄
Also, there's a quick merge conflict on docs/user-guide/rules/list.md, but it should have an easy resolution. I can do it, though I'm not sure if it invalidates my review.
|
We used to add code that show to user that some rules are deprecated: https://github.com/stylelint/stylelint/pull/2679/files#diff-29bacdfdd2b7605922ec3355625a795aR30-R35 We also have this docs about deprecation: https://stylelint.io/developer-guide/rules#deprecate-a-rule I think we need to add these warnings. What do you think? |
As discussed in issue stylelint#4854, the desired approach is to deprecate the *-blacklist/*-requirelist/*-whitelist rules. The first step I've taken to doing this is to make copies of all the rules. The commands used to generate this commit were: - find . -name '*-allowed-list' -exec sh -c 'cp -R "$1" "${1%-allowed-list}-whitelist"' _ {} \; - find . -name '*-disallowed-list' -exec sh -c 'cp -R "$1" "${1%-disallowed-list}-blacklist"' _ {} \; - find . -name '*-required-list' -exec sh -c 'cp -R "$1" "${1%-required-list}-requirelist"' _ {} \;
The aliasing approach taken for these rule renaming presented user experience problems [1]. To resolve these problems the preference is to revert on the aliasing strategy and instead return to a deprecation strategy. The first step towards this is re-instating these rules. As per the preference of jeddy3 [2] these rules are re-instated using a copy and paste strategy, with the expectation these will be removed in the next major release. An approach that didn't involve copy and pasting was previously introduced in [3] and could be looked at again if maintaining this duplicate code proves problematic. [1]: stylelint#4854 (comment) [2]: stylelint#4854 (comment) [3]: stylelint@e93e44c
This applies deprecation warnings when these rules are used, tests to check there is a deprecation warning and re-includes them in documentation.
kevindew
force-pushed
the
reinstate-deprecated-rules
branch
from
ac2e978
to
99877b6
Compare
|
Thanks @malsf21 - I just sorted the merge conflict.
Ack, I missed that documentation and copied a previous deprecation strategy. Anyway, we've got the warnings in place, see: stylelint/lib/rules/at-rule-blacklist/index.js Lines 32 to 35 in 99877b6 However, what seems a bit less clear to me is how I achieve the permanent GitHub link that is suggested in this PR, do you have an example? I couldn't find one. My dilemma is that once this branch is merged, the commits will be squashed so if I link directly to a commit (example) then it'll be part of a tree that is not a branch of this repository and - if there are any further rebases - potentially not part of this PR. However once this is merged the links could be updated to reference a merged commit, so I could open up a follow-up PR for that. Do let me know if you share concern on this and how you'd like me to proceed. |
No worries. Looks like we have to document it properly, because it's not documented.
It's not clear for me why it should be a link to Github, but in mentioned PR it's a lint to a our website ¯\_(ツ)_/¯ Anyway I came up with a solution — use tag view: https://github.com/stylelint/stylelint/blob/13.6.1/lib/rules/at-rule-property-requirelist/README.md We would need to use future version tag, which is not available now, but will be online when new version is released: https://github.com/stylelint/stylelint/blob/13.7.0/lib/rules/at-rule-property-requirelist/README.md |
If I remember correctly, it's because the website isn't versioned. When we remove the rules in the next major release, that will also remove the corresponding pages from the site because we build the website from the source files. The deprecation links will be 404ed. It's pretty clunky, but we never had time to build anything better.
Sounds good to me. @kevindew We've not deprecated any rules for a couple of years, and we're finding our way with this. Thanks for bearing with us. I've been busy with a new contract of late, but it's a Bank Holiday this weekend, and I should have time to get a release out. Will you have time to update the URLs to use the |
This reflects the documentation on deprecating stylelint rules [1] by linking to the GitHub website rather than stylelint website, so that the link can continue operating indefinitely. As suggested in the PR [2] these have been linked to the anticipated next release of stylelint, so these links will not be actually operational until this tag is made. [1]: https://github.com/stylelint/stylelint/blob/858dcd584224042654d80ce8fa8ad71f41f20808/docs/developer-guide/rules.md#deprecate-a-rule [2]: stylelint#4892 (comment)
|
Thanks @hudochenkov and @jeddy3 that tag suggestion works and I've applied it in 2794d6f. |
|
@kevindew Thanks for making the change. @hudochenkov The deprecation warnings look good to me know. Shall we merge? |
* master: (34 commits) Update CHANGELOG.md Fix double-slash disable comments when followed by another comment (#4913) Update CHANGELOG.md (#4916) 13.7.0 Prepare 13.7.0 Prepare changelog Update dependencies Update CHANGELOG.md Deprecate *-blacklist/*-requirelist/*-whitelist (#4892) Fix some path / glob problems (#4867) Update CHANGELOG.md Add a reportDescriptionlessDisables flag (#4907) Fix CHANGELOG.md format via Prettier (#4910) Fix callbacks in tests (#4903) Update CHANGELOG.md Fix false positives for trailing combinator in selector-combinator-space-after (#4878) Add coc-stylelint (#4901) Update CHANGELOG.md Add support for *.cjs config files (#4905) Add a reportDisables secondary option (#4897) ...
#4854
This takes the action suggested in @jeddy3's comment of replacing the aliasing approach used in #4845 for
*-blacklist/*-requirelist/*-whitelistrules with a deprecation approach. As suggested, this uses the copy and paste approach for rules rather than a factory approach to reduce code duplication. This is with the expectation that these rules will be removed in a soon major release and thus won't remain duplicated for long.As this was quite a manual task, the main issues to try catch in reviewing are copy+paste mistakes and anything that were missed. Given that this introduces a lot of copied and pasted code I've tried to make the reviewing easier by doing a bulk copy and paste in 4df7e92 and then my actual edits in the later commits. When reviewing, it'd be easiest to focus attention on the changes after the first commit.
This change should mean that stylelint/stylelint.io#159 no longer needs merging.