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 property-value-blacklist rule #689

Merged
merged 4 commits into from Feb 9, 2016

Conversation

@kangax
Copy link
Contributor

commented Feb 4, 2016

As discussed in #551

@davidtheclark

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2016

This looks like a solid start. Awesome.

Unfortunately, there's some additional complexity to address (things brought up in #551). Here's what comes to mind:

  • Values that contain multiple words, e.g. background-position: left center if I've blacklisted center. Might want to use postcss-value-parser to distinguish individual values and check each individually?
  • Shorthand values, e.g. You want to blacklist all for transitions: do you need to list transition: all and transition-property: all? Can't there be a better way? Seems we need to hit both or else there is an easy workaround for malicious violators. Makes sense in my mind that the configuration should specify transition-property: all, and the rule should look for the same keyword within transition.
  • When a value contains multiple words we probably need a regexp to check each one individually. This means we'd need a little clarification in the docs about what ^ and $ would mean in the context of these regeps.
  • What if the regex/string matches something within a url (e.g. blacklisted background-position: center and have background: url("/path/to/centerstage.gif");), or a string (can't think of a viable example .. do strings ever appear anywhere other than content -- which you just wouldn't blacklist?).

What do you think?

@kangax

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2016

Hey @davidtheclark. I feel like the points you bring up are all related to improved documentation.

Here's why:

Values that contain multiple words, e.g. background-position: left center if I've blacklisted center. Might want to use postcss-value-parser to distinguish individual values and check each individually?

This is an interesting point and I think it should simply be stated in the docs that the values are compared in their entirety. { "background-position": ["left"] } will only match when the value is exactly "left". For everything else there's regex (e.g. simple boundary-separated tokens ["/\bleft\b/", "/\bcenter\b/"]).

My reasoning is that if we bring heavier artillery like postcss-value-parser, then we open another decision can of worms: which tokens to match against? which separators? would I be able to match "1s" in something like "animation-duration: 1s, 2s, 3s" or only "animation-duration"? Do we match only function names or values? Do we match entire values or without units? And so on. These are all decisions that I'd rather not make and let users decide what they want. Regex gives decent flexibility, although of course it's not as reliable as a proper parser like postcss-value-parser is. In an ideal world, there would possibly be a support for a function (not just regex) which would be passed, say, a properly parsed AST of a value.

Shorthand values, e.g. You want to blacklist all for transitions: do you need to list transition: all and transition-property: all? Can't there be a better way? Seems we need to hit both or else there is an easy workaround for malicious violators. Makes sense in my mind that the configuration should specify transition-property: all, and the rule should look for the same keyword within transition.

Hm, sounds like we might want regex support for properties too then?

When a value contains multiple words we probably need a regexp to check each one individually. This means we'd need a little clarification in the docs about what ^ and $ would mean in the context of these regeps.

Agreed. We need to explain well in the docs what the regex is matching and where the boundaries are.

What if the regex/string matches something within a url (e.g. blacklisted background-position: center and have background: url("/path/to/centerstage.gif");), or a string (can't think of a viable example .. do strings ever appear anywhere other than content -- which you just wouldn't blacklist?).

There's a way to check with regex that a value is not within quotes. It's complex and not ideal, but other solutions are just as complex as far as I can see.

So that's my thoughts. What do you say? /cc @jeddy3

@davidtheclark

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2016

There's a way to check with regex that a value is not within quotes. It's complex and not ideal, but other solutions are just as complex as far as I can see.

Actually, there is one strategy that I've used before, which may seem hacky but does the trick: we could replace the argument of any url() in the value with an obscure special character -- preserving the value's original length while effectively removing the chance that something within that url will match the regex and cause a warning. I actually have a utility that I use in a couple of places called blurComments() that does just that but for comments (the use case every time is to make regexes easier). Does that seem like it might work for you? I wouldn't want users to have to write overly complex and error-prone regexes if they don't have to.

My reasoning is that if we bring heavier artillery like postcss-value-parser, then we open another decision can of worms

I guess that is true: although preferring regexes may not be the most user-friendly way to setup the rule, it is definitely the simplest; and given the amount of complexity and confusion that this rule invites I think I agree with you now that it's the way to go. Probably that means that the rule would benefit from some good examples in the docs (like the "/\\bleft\\b/" one you gave above).

sounds like we might want regex support for properties too then?

I guess so. We could just use the same convention for property names as for values: if the string starts and ends with /, it's parsed as a regex.

@kangax

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2016

@davidtheclark amended docs with regex clarification; fixed style errors

@davidtheclark

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2016

Cool. Looks good.

Here are the additional features I'd like before we release this rule --- you can work on them if you like or I can add them after merging this PR (unless you think we shouldn't add one):

  • Automatically ignore urls()
  • Allow and exemplify regexes in property names, so it's easy to address shorthands.
@kangax

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2016

Hm, I wouldn't ignore urls since that limits functionality (ignoring certain values in urls seems like a valid usecase). As for property regexes, that feels like a separate feature. Shouldn't it be allowed in all of the property-*-blacklist/whitelist rules?

@davidtheclark

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2016

I'm fine merging this PR after @jeddy3 reviews the docs.

I agree that property regexes can be considered a separate enhancement.

@kangax, since you provide at least one use-case that wants regexes to apply to urls, let's make ignoring urls() a secondary option: ignore: "url-arguments".

Again that can be a second PR. Happy to merge this work after @jeddy3 reviews the docs.

@jeddy3

This comment has been minimized.

Copy link
Member

commented Feb 9, 2016

let's make ignoring urls() a secondary option: ignore: "url-arguments".

Sgtm. I'll create an issue for it.

Happy to merge this work after @jeddy3 reviews the docs.

The readme looks good to me. I'll merge this PR in now and do the wiring and docs. @kangax For future reference each new rule is accompanied by changes to:

Thanks again to you both for thrashing this rule out! :)

jeddy3 added a commit that referenced this pull request Feb 9, 2016

Merge pull request #689 from kangax/master
Add property-value-blacklist rule

@jeddy3 jeddy3 merged commit 7f03e8a into stylelint:master Feb 9, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kangax

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2016

Awesome, thanks! And yes, I forgot about docs again :)

On Tue, Feb 9, 2016 at 8:44 AM, Richard Hallows notifications@github.com
wrote:

Merged #689 #689.


Reply to this email directly or view it on GitHub
#689 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.