Replace -whitelist/-blacklist/-requirelist rules with -allowed-list/-disallowed-list/-required-list rules #4845

kevindew · 2020-06-26T08:42:18Z

Which issue, if any, is this issue related to?

Resolves #4844

Is there anything in the PR that needs further explanation?

This PR serves as a potential implementation route to achieve the re-naming change raised in issue #4844, where I expect there is still to be a reasonable amount of discussion on the issue. It has been reworked following @jeddy3 suggestion on an implementation route.

It takes an approach of renaming all *-blacklist, *-requirelist and *-whitelist rules to *-disallowed-list, *-required-list and *-allowed-list which creates a consistent *-list pattern for naming list based rules. There have then been aliases created so that the previously named rules remain operational.

jeddy3 · 2020-06-27T15:33:42Z

@kevindew Thank you for getting the ball rolling with this.

The approach I took for this was to rename the existing rules first (and associated documentation), so that the git history on the rules could be transferred to the new rules

Excellent. Regardless of the final route we decide on, we'll need to go about it this way.

This PR serves as a potential implementation route to achieve the re-naming change raised in issue #4844, where I expect there is still to be a reasonable amount of discussion on the issue.

I've made some suggestions about the implementation route and renaming over in the issue.

This applies a full rename across the repository to reflect that the term "blacklist" is controversial in the tech community and that using the term "disallowed-list" is more explicit in it's purpose.

kevindew · 2020-06-28T12:11:56Z

@kevindew Thank you for getting the ball rolling with this.

The approach I took for this was to rename the existing rules first (and associated documentation), so that the git history on the rules could be transferred to the new rules

Excellent. Regardless of the final route we decide on, we'll need to go about it this way.

This PR serves as a potential implementation route to achieve the re-naming change raised in issue #4844, where I expect there is still to be a reasonable amount of discussion on the issue.

I've made some suggestions about the implementation route and renaming over in the issue.

Thanks, the clarity in your suggestions was super helpful. I've now amended this in line with them, do let me know if I've misinterpreted any.

I wasn't too sure of what testing approach, if any, to take with the aliased rules and put this together where I might benefit from some pointers to precedence if it's not sufficiently consistent with this codebase.

I also wasn't sure if this risks being a BC break given that violations for a whitelist rule will be returned under a new rule name. For example, a unit-whitelist violation will be raised as a unit-allowed-list violation. I expect you'll know better than me if that is considered part of the API.

jeddy3

@kevindew Thanks for putting all the legwork into this one. It's looking great.

I've requested:

some very minor nitpicks
the regeneration of the example.png screenshot

I wasn't too sure of what testing approach,

Tests look great.

I also wasn't sure if this risks being a BC break given that violations for a whitelist rule will be returned under a new rule name.

I think we're good. The only place outside of a config where the rule names are used is within the checkAgainstRule plugin utility. It would be great if we could add a test ensuring the renamed rule is in the warning when an alias is used.

jeddy3 · 2020-06-29T11:31:17Z

docs/user-guide/rules/list.md

- [`function-url-scheme-blacklist`](../../../lib/rules/function-url-scheme-blacklist/README.md): Specify a blacklist of disallowed URL schemes.
- [`function-url-scheme-whitelist`](../../../lib/rules/function-url-scheme-whitelist/README.md): Specify a whitelist of allowed URL schemes.
- [`function-whitelist`](../../../lib/rules/function-whitelist/README.md): Specify a whitelist of allowed functions.
+- [`function-url-scheme-disallowed-list`](../../../lib/rules/function-url-scheme-disallowed-list/README.md): Specify a list of disallowed URL schemes.


Let's alphabetize this and the next line.

Whoops, nice spot 👍

jeddy3 · 2020-06-29T11:41:45Z

lib/__tests__/integration.test.js

@@ -141,6 +143,19 @@ it('Scss integration test', () => {
 		});
 });

+it('Rule aliasing integration test', () => {


Suggested change

it('Rule aliasing integration test', () => {

it('rule aliasing integration test', () => {

Let's use a lower case here to be consistent with the majority of other descriptions (the capitalised "Scss..." description above is more an exception than the norm).

jeddy3 · 2020-06-29T11:44:41Z

lib/rules/at-rule-property-required-list/index.js


 const messages = ruleMessages(ruleName, {
 	expected: (property, atRule) => `Expected property "${property}" for at-rule "${atRule}"`,
 });

-function rule(primary) {
+function rule(list) {


Thanks for making this consistent.

jeddy3 · 2020-06-29T11:49:05Z

lib/rules/declaration-property-unit-disallowed-list/index.js

@@ -12,16 +12,16 @@ const ruleMessages = require('../../utils/ruleMessages');
 const validateOptions = require('../../utils/validateOptions');


This file shifts from whitelist to disallowed list, and vice-versa for its twin. However, the logic also shifts in both rules, so things work out in the end.

I think we can roll with it, rather than try to unpick it.

This just seems to be GitHub becoming confused when merging the diffs of multiple commits. In the individual commits this rule doesn't shift.For example:

rename lib/rules/{declaration-property-unit-blacklist => declaration-property-unit-disallowed-list}/README.md (91%) rename lib/rules/{declaration-property-unit-blacklist => declaration-property-unit-disallowed-list}/__tests__/index.js (100%) rename lib/rules/{declaration-property-unit-blacklist => declaration-property-unit-disallowed-list}/index.js (85%)

and

rename lib/rules/{declaration-property-unit-whitelist => declaration-property-unit-allowed-list}/README.md (92%) rename lib/rules/{declaration-property-unit-whitelist => declaration-property-unit-allowed-list}/__tests__/index.js (100%) rename lib/rules/{declaration-property-unit-whitelist => declaration-property-unit-allowed-list}/index.js (85%)

jeddy3 · 2020-06-29T11:57:42Z

lib/rules/selector-pseudo-element-allowed-list/README.md

@@ -1,6 +1,6 @@
-# selector-pseudo-element-blacklist


A blacklist to allowed list shift, but the examples shifted too. As with the rule shift above, I think we're good to roll with this.

Same GitHub session, looks good in commit history.

jeddy3 · 2020-06-29T12:08:16Z

scripts/visual-config.json

@@ -1,7 +1,7 @@
 {
 	"rules": {
 		"color-no-invalid-hex": true,
-		"declaration-property-unit-blacklist": { "margin": ["px"] },
+		"declaration-property-unit-disallowed-list": { "margin": ["px"] },


Suggested change

"declaration-property-unit-disallowed-list": { "margin": ["px"] },

"unit-disallowed-list": ["px"],

Let's:

change this to the shorter unit-disallowed-list

run cd scripts && ./visual.sh

screenshot the "Normal" terminal output

optimise the resultant .png

replace https://github.com/stylelint/stylelint/blob/master/example.png with it

It's shown in the README and project homepage.

Cool, I've added this in 8e8aa2c

This applies a full rename across the repository to reflect that the term "whitelist" is controversial in the tech community and that using the term "allowed-list" is more explicit in it's purpose.

This is to make this rule consistent in naming with the other list based rule names (*-allowed-list and *-disallowed-list).

Previously *-whitelist, *-blacklist and *-requirelist rules were renamed to a consistent naming pattern of *-allowed-list, *-disallowed-list and *-required-list respectively. This commit adds aliases so that the old rules can continue to operate; this allows backwards compatibility to be maintained. A consequence of this is that violations for the previously named rules will show up with the new name. A developer may also get some confusing results if they happen to have the same rule defined with both names and different options.

This replaces the declaration-property-unit-disallowed-list rule with the shorter unit-disallowed-unit one to create a simpler example image.

kevindew · 2020-06-30T20:07:11Z

I think we're good. The only place outside of a config where the rule names are used is within the checkAgainstRule plugin utility. It would be great if we could add a test ensuring the renamed rule is in the warning when an alias is used.

I've added that in 96acce6 with:

stylelint/lib/utils/__tests__/checkAgainstRule.test.js

Lines 64 to 82 in 96acce6

    
           it('warns against a renamed rule when an aliased rule is used', () => { 
        
           	const root = postcss.parse('a { top: 10px; }'); 
        
           	const warnings = []; 
        
           	checkAgainstRule( 
        
           		{ 
        
           			ruleName: 'unit-whitelist', 
        
           			ruleSettings: ['em'], 
        
           			root, 
        
           		}, 
        
           		(warning) => warnings.push(warning), 
        
           	); 
        
           	expect(warnings).toHaveLength(1); 
        
           	expect(warnings[0].rule).toBe('unit-allowed-list'); 
        
           	expect(warnings[0].line).toBe(1); 
        
           	expect(warnings[0].column).toBe(10); 
        
           });

Let me know if you'd like any amends to it (I don't have much of an understanding of the code this part is testing)

jeddy3

Excellent, thank you! It all looks good to me now.

mattxwang

Ran your fork locally and looked through the code, everything looks good to me! This is a large change, thank you for all the work! LGTM!

Would like to loop in a more experienced member on the @stylelint/core team, since this is a pretty big change.

Also, have we made the PR to the stylelint.io website to add redirects for the renamed rules and/or a quick writeup explaining what we did? We'll need to do that before releasing, right?

(and update configs: https://github.com/stylelint/stylelint-config-standard
https://github.com/stylelint/stylelint-config-recommended )

kevindew · 2020-07-03T08:08:33Z

Great stuff, yup this needs at least a changelog entry for this repo. Would that be where the write-up goes or somewhere else?

I’ve not opened any other PRs in other repos but I’ll look into sorting that out now this is on the way in.

mattxwang · 2020-07-03T08:13:57Z

Great stuff, yup this needs at least a changelog entry for this repo. Would that be where the write-up goes or somewhere else?

At least in the CHANGELOG.md for sure, though I'd also imagine that something on the website or more user-facing would also be needed? I'm still pretty new around here, so I'd defer to @jeddy3.

I’ve not opened any other PRs in other repos but I’ll look into sorting that out now this is on the way in.

Sounds good! I can help you with some of the low-hanging but tedious fruit like the config updating!

jeddy3 · 2020-07-03T10:18:18Z

Also, have we made the PR to the stylelint.io website to add redirects for the renamed rules?

Not yet. It's the only thing remaining to do.

I can help you with some of the low-hanging but tedious fruit like the config updating!

@malsf21 Excellent. If you've time, would you mind rustling up a pull request for the redirects? I've created an issue for it: stylelint/stylelint.io#158

Great stuff, yup this needs at least a changelog entry for this repo.

We'll add a changelog entry just after merging. We'll tweet the release too.

jeddy3 · 2020-07-06T15:03:08Z

Changelog:

Added: *-allowed-list, *-disallowed-list and *-required-list new names for *-whitelist, *-blacklist and *-requirelist rules, respectively; the rules are aliased as their old names (#4845).

# By Richard Hallows (4) and others # Via GitHub * master: Create codeql analysis workflow (#4850) Update CHANGELOG.md Add support for descriptions in stylelint command comments (#4848) Update CHANGELOG.md Add *-allowed-list/*-disallowed-list/*-required-list names for *-whitelist/*-blacklist/*-requirelist rules (#4845) Update CHANGELOG.md Add syntax object acceptance to customSyntax option (#4839) Bump typescript from 3.9.5 to 3.9.6 (#4853) Add ignoreComments[] to comment-empty-line-before (#4841) Bump autoprefixer from 9.8.0 to 9.8.2 (#4838)

Due to stylelint/stylelint#4845

kevindew mentioned this pull request Jun 26, 2020

Add new names and alias *-*list rules #4844

Closed

Rename *-blacklist rules to *-disallowed-list

a688fec

This applies a full rename across the repository to reflect that the term "blacklist" is controversial in the tech community and that using the term "disallowed-list" is more explicit in it's purpose.

mattxwang mentioned this pull request Jun 28, 2020

Add function-name-arguments-allowed-list #4816

Closed

kevindew force-pushed the allowlist-denylist branch from 28e1a31 to cbdf5de Compare June 28, 2020 12:01

kevindew changed the title ~~Deprecate *-whitelist/*-blacklist rules in favour of *-allowlist/*-denylist~~ Replace *-whitelist/*-blacklist/*-requirelist rules with *-allowed-list/*-disallowed-list/*-required-list rules Jun 28, 2020

jeddy3 requested changes Jun 29, 2020

View reviewed changes

kevindew added 4 commits June 30, 2020 20:17

Rename *-whitelist rules to *-allowed-list

1472f51

This applies a full rename across the repository to reflect that the term "whitelist" is controversial in the tech community and that using the term "allowed-list" is more explicit in it's purpose.

Rename requirelist rule to required-list

3f7b169

This is to make this rule consistent in naming with the other list based rule names (*-allowed-list and *-disallowed-list).

Regenerate example.png

8e8aa2c

This replaces the declaration-property-unit-disallowed-list rule with the shorter unit-disallowed-unit one to create a simpler example image.

kevindew force-pushed the allowlist-denylist branch from cbdf5de to 8e8aa2c Compare June 30, 2020 20:01

jeddy3 approved these changes Jul 1, 2020

View reviewed changes

mattxwang approved these changes Jul 3, 2020

View reviewed changes

This was referenced Jul 3, 2020

Replace *-whitelist/*-blacklist rules with *-allowed-list/*-disallowed-list rules stylelint/stylelint-config-standard#150

Closed

Replace *-whitelist/*-blacklist rules with *-allowed-list/*-disallowed-list rules stylelint/stylelint-config-standard#151

Merged

This was referenced Jul 5, 2020

Replace unit-whitelist with unit-allowed-list stylelint/stylelint-config-recommended#69

Closed

Replace unit-whitelist with unit-allowed-list in README example stylelint/stylelint-config-recommended#70

Merged

jeddy3 mentioned this pull request Jul 6, 2020

Add redirects for *list rules stylelint/stylelint.io#159

Closed

jeddy3 merged commit 6c48085 into stylelint:master Jul 6, 2020

sarayourfriend mentioned this pull request Jul 8, 2020

Remove problematic language Automattic/wp-calypso#43395

Closed

21 tasks

jeddy3 mentioned this pull request Aug 8, 2020

Release 13.7.0 #4854

Closed

6 tasks

kevindew mentioned this pull request Aug 8, 2020

Deprecate *-blacklist/*-requirelist/*-whitelist #4892

Merged

marekdedic mentioned this pull request Sep 2, 2020

declaration-property-unit-whitelist has been deprecated WordPress-Coding-Standards/stylelint-config-wordpress#266

Closed

miktirr mentioned this pull request Oct 13, 2020

Upgrade stylelint version to support the new rule names gilbarbara/codeclimate-stylelint#44

Closed

This was referenced Mar 8, 2021

Bump stylelint from 13.6.1 to 13.12.0 mfuerni/Catroweb#404

Closed

Bump stylelint from 13.6.1 to 13.12.0 stevewithington/bootstrap#83

Closed

dependabot bot mentioned this pull request Mar 15, 2021

Bump stylelint from 11.1.1 to 13.12.0 statisticsnorway/ssb-component-library#255

Merged

adalinesimonian mentioned this pull request Sep 30, 2021

Rename default branch to main stylelint/vscode-stylelint#245

Closed

3 tasks

mountainash added a commit to mountainash/wordpressify that referenced this pull request May 3, 2022

"Political correctness" fix

960de36

Due to stylelint/stylelint#4845

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace -whitelist/-blacklist/-requirelist rules with -allowed-list/-disallowed-list/-required-list rules #4845

Replace -whitelist/-blacklist/-requirelist rules with -allowed-list/-disallowed-list/-required-list rules #4845

kevindew commented Jun 26, 2020 •

edited

jeddy3 commented Jun 27, 2020

kevindew commented Jun 28, 2020

jeddy3 left a comment

jeddy3 Jun 29, 2020

kevindew Jun 30, 2020

jeddy3 Jun 29, 2020

kevindew Jun 30, 2020

jeddy3 Jun 29, 2020

jeddy3 Jun 29, 2020

kevindew Jun 30, 2020

jeddy3 Jun 29, 2020

kevindew Jun 30, 2020

jeddy3 Jun 29, 2020

kevindew Jun 30, 2020

kevindew commented Jun 30, 2020

jeddy3 left a comment

mattxwang left a comment •

edited

kevindew commented Jul 3, 2020

mattxwang commented Jul 3, 2020

jeddy3 commented Jul 3, 2020

jeddy3 commented Jul 6, 2020

	it('Rule aliasing integration test', () => {
	it('rule aliasing integration test', () => {

		@@ -12,16 +12,16 @@ const ruleMessages = require('../../utils/ruleMessages');
		const validateOptions = require('../../utils/validateOptions');

	"declaration-property-unit-disallowed-list": { "margin": ["px"] },
	"unit-disallowed-list": ["px"],

Replace *-whitelist/*-blacklist/*-requirelist rules with *-allowed-list/*-disallowed-list/*-required-list rules #4845

Replace *-whitelist/*-blacklist/*-requirelist rules with *-allowed-list/*-disallowed-list/*-required-list rules #4845

Conversation

kevindew commented Jun 26, 2020 • edited

jeddy3 commented Jun 27, 2020

kevindew commented Jun 28, 2020

jeddy3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindew commented Jun 30, 2020

jeddy3 left a comment

Choose a reason for hiding this comment

mattxwang left a comment • edited

Choose a reason for hiding this comment

kevindew commented Jul 3, 2020

mattxwang commented Jul 3, 2020

jeddy3 commented Jul 3, 2020

jeddy3 commented Jul 6, 2020

Replace -whitelist/-blacklist/-requirelist rules with -allowed-list/-disallowed-list/-required-list rules #4845

Replace -whitelist/-blacklist/-requirelist rules with -allowed-list/-disallowed-list/-required-list rules #4845

kevindew commented Jun 26, 2020 •

edited

mattxwang left a comment •

edited