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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ignore:["custom-element"] option to selector-type-no-unknown #2366

Merged
merged 1 commit into from Apr 16, 2017

Conversation

5 participants
@evilebottnawi
Member

evilebottnawi commented Feb 16, 2017

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

#2348

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

Btw, i think be good add option resolve: ['custom-element'] (maybe best name). Because not-my-type is valid as custom element but invalid as known type selector 馃槶

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Feb 16, 2017

Can you update the docs:

This rule considers tags defined in the HTML and SVG Specifications to be known.

Becomes:

This rule considers tags which are valid custom elements and those defined in the following Specifications to be known: HTML and SVG.

(We'll likely be adding MathML to that list).

Can you also update the examples not to use hyphens.

Lastly, can you remove the hyphen from the regex tests to make them relevant again.

Btw, i think be good add option resolve: ['custom-element'] (maybe best name). Because not-my-type is valid as custom element but invalid as known type selector

Can you reword this please? It's not clear what you mean.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Feb 17, 2017

@jeddy3 if we merge this PR type selector foo-bar be valid type selector (because it is valid as custom selector), if we want avoid this situation we should add option resolve: ["custom-selector"] (maybe best), if option enable foo-bar is valid as type selector, if option disable foo-bar is invalid as type selector, sorry for my bad english

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Feb 17, 2017

Oh I see. You're suggesting we put this behind an option e.g. ignore: ["custom-elements"]?

if option enable foo-bar is valid as type selector, if option disable foo-bar is invalid as type selector

That would address this issue.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Feb 17, 2017

@jeddy3 yep, it will be good solution

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 18, 2017

I'm wondering if this option should be reversed. Instead of by default complaining about custom elements, it seems we should by default let them pass. Maybe we'd add a disallowCustomElements option to get the reverse.

(It's probably unlikely that a legitimate custom element is going to be confused with a type selector typo, because custom elements require hyphens and almost no real type selectors do.)

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Feb 18, 2017

I'm wondering if this option should be reversed. Instead of by default complaining about custom elements, it seems we should by default let them pass.

I agree.

Maybe we'd add a disallowCustomElements option to get the reverse.

Yep, but I think we can wait until someone actually runs into the scenario and requests it (as, like you said, it seems unlikely to occur).

So, I think this PR is ready to merge once @evilebottnawi addresses the doc and test requests.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Feb 23, 2017

@jeddy3 some my project use custom elements some no, i am add this option 馃槃 , i requests this 馃槃

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Feb 26, 2017

some my project use custom elements some no, i am add this option 馃槃 , i requests this

Sounds good. Let's create a separate issue for that and add that option in a separate PR. Let's do the doc and test requests and merge this.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Mar 15, 2017

@evilebottnawi Do you plan to finish this PR, or should we close it?

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 15, 2017

@davidtheclark finish on this week 馃槃

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 21, 2017

/cc @jeddy3 Done

@jeddy3

jeddy3 approved these changes Mar 25, 2017 edited

LGTM thanks!

@davidtheclark

One suggestion, otherwise 馃憤

* Check whether a type selector is a custom element
*/
module.exports = function (selector/*: string*/)/*: boolean*/ {
const startWithALowercaseASCII = (/^[a-z]/m).test(selector)

This comment has been minimized.

@davidtheclark

davidtheclark Mar 25, 2017

Contributor

Why the m flag?

This comment has been minimized.

@evilebottnawi
&& containAHyphen
&& isNotSvgTag
&& isNotHtmlTag
&& isNotMathmlTag

This comment has been minimized.

@davidtheclark

davidtheclark Mar 25, 2017

Contributor

This is kind of a weird way to structure the function. Couldn't you have a return after each statement and avoid the need to assign variables? E.g.

if (!(/^[a-z]/m).test(selector)) return false
if (selector.toLowerCase() !== selector) return false
...

This comment has been minimized.

@evilebottnawi
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Mar 26, 2017

I'm wondering if this option should be reversed. Instead of by default complaining about custom elements, it seems we should by default let them pass. Maybe we'd add a disallowCustomElements option to get the reverse.

I just realised, while thinking through kristerkari/stylelint-scss#103 (comment), that perhaps this behaviour should be behind an option. Sorry to flip-flop.

I believe every no rule in stylelint is most restrictive by default and can be made more permissive using "ignore": [] options. We even changed indentation to be consistent with this behaviour a while back, but checking function parameters by default (and providing an ignore option) rather than via opt-in.

I think adding a disallow... boolean option would be at odds with what we've done elsewhere... I think it's also the reason #2437 confused me - that is the corresponding issue for this disallow... option, right? But it looks like @evilebottnawi instinctively, and understandably so, reach for an ignore* option.

Putting this behind an option also fits with the idea of "known" being a discrete list of elements from specifications, whereas ignore: ["custom-elements"] is triggering a heuristic ignore.

Want do you think?

@jeddy3

Move behind option?

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 27, 2017

@jeddy3 sound good for option 馃憤

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Apr 1, 2017

sound good for option

@evilebottnawi Is this change something you can factor into this PR?

@jeddy3 jeddy3 changed the title from Enhancement: ignore Custom Elements by default. to Add ignore:["custom-elements' Apr 1, 2017

@jeddy3 jeddy3 changed the title from Add ignore:["custom-elements' to Add ignore:["custom-element"] option to selector-type-no-unknown Apr 1, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 1, 2017

@jeddy3 yep, in near future

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 7, 2017

@jeddy3

Looking good. Just 3 very minor nit-picks.

@@ -97,10 +74,20 @@ const rule = function (actual, options) {
return
}
if (

This comment has been minimized.

@jeddy3

jeddy3 Apr 7, 2017

Member

Minor request... can we get this shifted to line 61 so the early returns are in the same alphabetical order as the options config.

Quite a lot of rules now have 5 or more ignore/except* options and I think there's no harm adding a bit of consistent structure to them.

const rule = function (actual, options) {
return (root, result) => {
const validOptions = validateOptions(result, ruleName, { actual }, {
actual: options,
possible: {
ignore: [
"default-namespace",
"custom-elements",

This comment has been minimized.

@jeddy3

jeddy3 Apr 7, 2017

Member

Alphabetical order please.

@@ -8,7 +8,7 @@ Disallow unknown type selectors.
* This type selector */
```
This rule considers tags defined in the HTML and SVG Specifications to be known.
This rule considers tags which are valid custom elements and those defined in the following Specifications to be known: HTML and SVG.

This comment has been minimized.

@jeddy3

jeddy3 Apr 7, 2017

Member

Rebase from master and go with what's in #2478

@evilebottnawi evilebottnawi force-pushed the issue-2348 branch from fff628f to dab2a02 Apr 7, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 7, 2017

/cc @jeddy3

@jeddy3

jeddy3 approved these changes Apr 7, 2017

LGTM

@awebdeveloper

This comment has been minimized.

Contributor

awebdeveloper commented Apr 12, 2017

is this merged. Can a new tag be created

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 12, 2017

@awebdeveloper wait after @davidtheclark accept.

@awebdeveloper

This comment has been minimized.

Contributor

awebdeveloper commented Apr 12, 2017

@evilebottnawi just to clarify will this work for all the below cases

1. my-element
2. element
3. [my-attr]

@davidtheclark davidtheclark merged commit 95c6b17 into master Apr 16, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 95.474%
Details

@davidtheclark davidtheclark deleted the issue-2348 branch Apr 16, 2017

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Apr 16, 2017

Added to changelog:

  • Fixed: selector-type-no-unknown now ignores valid custom elements (#2366).
@abdonrd

This comment has been minimized.

abdonrd commented Apr 16, 2017

Thanks! 馃憦 馃帀

@awebdeveloper

This comment has been minimized.

Contributor

awebdeveloper commented Apr 16, 2017

looking at the docs this doesn't take into account custom attributes. [my-attr]. Maybe a different PR but that should also be included

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 17, 2017

@awebdeveloper please create issue with template for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment