Add rule: class-name-format #327

benthemonkey · 2015-10-18T02:59:50Z

Slightly different than the other name-format rules. It adds ignore, an array of whitelisted names.

Add allow-leading-underscore back in
Add rule to lib/config/sass-lint.yml
Still need to add unit tests for the two helper functions I added.
More test examples
Handle suffix extensions to selectors

I welcome feedback on the documentation, as I'm not very familiar with BEM.

Will close #324

DCO 1.1 Signed-off-by: Ben Rothman bensrothman@gmail.com

DanPurdy · 2015-10-18T07:28:40Z

I think this needs a lot more test examples, including parent selectors etc, I'll add some examples here later on.

bgriffith · 2015-10-18T10:32:51Z

@benthemonkey Just a heads up that you need to add the rule to lib/config/sass-lint.yml for each new rule you create 👍

Snugug · 2015-10-18T11:35:52Z

And needs to allow a leading underscore. They are valid first characters for class names (and something I personally use)

On Oct 17, 2015, at 10:59 PM, Ben Rothman notifications@github.com wrote:

Slightly different than the other name-format rules. It doesn't include allow-leading-underscore and it adds ignore, an array of whitelisted names.

Still need to add unit tests for the two helper functions I added.

Will close #324

DCO 1.1 Signed-off-by: Ben Rothman bensrothman@gmail.com

You can view, comment on, or merge this pull request online at:

#327

Commit Summary

🔍 Add class-name-format rule and accompanying tests
File Changes

A docs/rules/class-name-format.md (205)
M lib/helpers.js (8)
A lib/rules/class-name-format.js (74)
A tests/rules/class-name-format.js (171)
A tests/sass/class-name-format.sass (40)
A tests/sass/class-name-format.scss (53)
A tests/yml/.merge-default-rules-false.yml (2)
Patch Links:

https://github.com/sasstools/sass-lint/pull/327.patch
https://github.com/sasstools/sass-lint/pull/327.diff
—
Reply to this email directly or view it on GitHub.

BPScott · 2015-10-18T12:45:17Z

It looks like the regexs are also missing the escape sequences that the CSS Spec for identifiers defines.

In CSS, identifiers (including element names, classes, and IDs in selectors) can contain only the characters [a-zA-Z0-9] and ISO 10646 characters U+00A0 and higher, plus the hyphen (-) and the underscore (_); they cannot start with a digit, two hyphens, or a hyphen followed by a digit. Identifiers can also contain escaped characters and any ISO 10646 character as a numeric code (see next item). For instance, the identifier "B&W?" may be written as "B&W?" or "B\26 W\3F".

You can take a look at the CSS Grammar for a regex-like interpretation of that rule. Mathias Bynens has a primer on CSS escapes with a test suite of pathologically absurd-but-still-valid class names.

Long story short, this sort of thing is valid CSS class naming.

Snugug · 2015-10-18T12:52:10Z

In a total unrelated note, how many Bens are working on SassLint now? Three Bens, a Dan, and a Sam?

On Oct 18, 2015, at 8:45 AM, Ben Scott notifications@github.com wrote:

It looks like the regexs are also missing the escape sequences that the CSS Spec for identifiers defines.

In CSS, identifiers (including element names, classes, and IDs in selectors) can contain only the characters [a-zA-Z0-9] and ISO 10646 characters U+00A0 and higher, plus the hyphen (-) and the underscore (_); they cannot start with a digit, two hyphens, or a hyphen followed by a digit. Identifiers can also contain escaped characters and any ISO 10646 character as a numeric code (see next item). For instance, the identifier "B&W?" may be written as "B&W?" or "B\26 W\3F".

You can take a look at the CSS Grammar for a regex-like interpretation of that rule. Mathias Bynens has a primer on CSS escapes with a test suite of pathologically absurd-but-still-valid class names.

Long story short, this sort of thing is valid CSS class naming.

—
Reply to this email directly or view it on GitHub.

bgriffith · 2015-10-18T13:05:08Z

haha world domination soon 😛

benthemonkey · 2015-10-18T16:17:09Z

OK I'll definitely have the class naming adhere to the spec. These regexp's were grabbed straight from scss-lint (as I did with previous rules). But now that you mention it I've wasted hours in the past figuring out why my CSS selector wasn't working, and the problem was an ID started with a number.

However, do you think this rule should perform the role of class validity checking? The alternative is to differentiate rules which check for style from rules which check for syntax issues / validity (again, ESLint's model).

For example, if I had .101_dalmatians, should the linter fire two warnings? "Class '.101_dalmatians' should be written in lowercase with hyphens" and "Class names cannot begin with a digit"

DanPurdy · 2015-10-18T16:29:07Z

I would say in the spirit of small single purpose rules we're aiming for we should probably stick with the validity of classnames being handled by a separate rule.

benthemonkey · 2015-10-18T17:49:15Z

I feel like the documentation for this rule is unnecessarily long. Does the user really need to see Good/Bad examples of every convention? Can't I just briefly describe the format of each convention (possibly in a separate documentation page to avoid duplication)? I feel like it would be more concise and consumable by the reader. I would still include examples for the other options.

For instance:

Convention Examples

hyphenatedlowercase
- header
- header-title
- header-title-active
camelcase
- header
- headerTitle
- headerTitleActive
hyphenatedbem
- header
- header__title
- header--inverted
- header--inverted__title

benthemonkey · 2015-10-18T18:13:48Z

@BPScott I don't think that escape sequences or number codes, while valid, fit within any of the conventions included in this rule. E.g. if a user chooses hyphenatedlowercase, I believe they are implicitly allowing only a-z, 0-9 and hyphens.

BPScott · 2015-10-18T18:31:57Z

In a total unrelated note, how many Bens are working on SassLint now? Three Bens, a Dan, and a Sam?

I refuse to believe my rocking up in the issues and muttering "you missed a bit" counts as working on sass-lint, everyone else contributing code is easily in a different league ;)

@benthemonkey: Good point. The escape code stuff is very much a niche thing and should be avoided 99% of the time. Having this rule for naming conventions with the current regexes, and then a new lower-level rule for no-invalid-selectors that deals with the stricter notion of a valid selector - funky escape codes and all - sounds like a good plan.

Then a user can leave the validity check on for a whole codebase, while occasionally explicitly ignoring the naming convention rule for when they do want a class using escape codes using #70.

DanPurdy · 2015-10-18T22:01:42Z

So for a start with BEM you'll want to check for the following which should all be valid

.block
.block__element
.block__element--modifier
.block--modifier

.block {
  &__element {

    &--mofifier {

    }
  }
  &--modifier {

  }
}

benthemonkey · 2015-10-24T22:40:25Z

@DanPurdy So I tried out your example, utilizing suffix additions to selectors. Gonzales-PE doesn't consider those suffix additions to be part of a class name, and so they are ignored by this linter.

Do you know if I basically have to wait for us to version bump Gonzales-PE so I can utilize the parentSelectorExtension node?

DanPurdy · 2015-10-24T22:46:03Z

It should see them as parent selectors no? Looking at your code you're parsing by class, if you were to traverse by selectors as I had to in no-mergeable-selectors you'll get much more information. The question then is whether we care about these parent selector additions and whether parsing those is out of scope of actual class name format.

benthemonkey · 2015-10-31T06:17:55Z

Sorry for such a long delay. I played around with accomplishing this, and it was just way too complicated. Here's the location of a class selector, relative to a suffix addition to the selector:

Class selector:
ruleset -> selector -> simpleSelector -> class -> ident
Suffix addition
ruleset -> block -> ruleset -> selector -> simpleSelector -> (parentSelector and ident)

That's crazy hard to dependably reason about, right? I didn't expect the block of a style definition to be a sibling to the selector, instead of a child of the selector. Not to mention the fact that this has to be recursive, so that multiple nested suffix additions can be processed. Maybe I'm just still getting used to tree traversals. Do you think this can reasonably be tested for?

Edit: @DanPurdy

benthemonkey · 2015-11-09T21:27:21Z

Poking @sasstools/sass-lint-contributors to try to close up this PR. Are you all ok with not supporting suffix extensions to selectors in the initial version of this rule? If not, could you provide some insight into the best way to robustly accomplish the tree traversal I describe in my previous comment?

DanPurdy · 2015-11-11T16:01:27Z

tests/yml/.merge-default-rules-false.yml

@@ -0,0 +1,2 @@
+options:
+  merge-default-rules: false


Any reason for this to be there in this PR?

DanPurdy · 2015-11-11T16:06:38Z

So we're saying it wouldnt support parent selectors?

benthemonkey · 2015-11-11T16:45:36Z

We're going to have to support suffix extensions to selectors when we write a bem-depth rule, so I'll give this another shot. I think I'll need to write some tree-traversal helper functions to accomplish this. Maybe a searchForSelectorExtensions function would be nice.

This is a more global issue than I realized. Any selector can be extended so a lot of our rules need fixing.

DanPurdy · 2015-11-11T16:57:06Z

I'm just trying to get at what we're classing as suffix extensions here. If it's all parent selectors then I've already sort of gone through that process with the no-mergeable-selectors rule that pretty much constructs every class name correctly though it doesn't format them into a usable format for this rule but you could get close to it.

benthemonkey · 2015-11-11T17:11:01Z

By "suffix extensions to selectors," I was trying to mimic Sass' documentation for the best way to describe adding stuff to the end of a parent's class name. "& must appear at the beginning of a compound selector, but it can be followed by a suffix that will be added to the parent selector" source. For our conversations I thought it would be helpful to have a term to describe this specific use of parent selectors.

I'll look more carefully at no-mergeable-selectors.

benthemonkey · 2015-11-12T06:33:11Z

@DanPurdy Quick question. For BEM stuff, once we check suffix extensions to selectors, do we allow stuff like this?

.block_ {
  &_element-- {
    &modifier {}
  }
}

Because the way I'm currently writing this rule, I don't know how I should check for that sort of thing...

DanPurdy · 2015-11-17T17:43:36Z

lib/helpers.js

+    yaml = require('js-yaml'),
+    merge = require('merge');
+
+var clone = function (obj) {


Can we include JSDoc style comments for each new function in helpers please.

…owed

benthemonkey · 2015-11-17T20:01:08Z

@DanPurdy I think I covered all your comments

DanPurdy · 2015-11-19T11:55:32Z

👍

Add rule: class-name-format

benthemonkey added rule in progress labels Oct 18, 2015

benthemonkey mentioned this pull request Oct 18, 2015

New Rule: Selector Validity #328

Open

benthemonkey force-pushed the feature/class-name-format branch 2 times, most recently from b07aa1b to 95e9dc9 Compare October 24, 2015 22:11

benthemonkey force-pushed the feature/class-name-format branch from 95e9dc9 to 95bf03f Compare October 30, 2015 06:10

DanPurdy reviewed Nov 11, 2015
View reviewed changes

benthemonkey force-pushed the feature/class-name-format branch from 95bf03f to 66c3b93 Compare November 11, 2015 17:12

benthemonkey force-pushed the feature/class-name-format branch from 9f5201f to 6ff1a42 Compare November 12, 2015 09:21

DanPurdy reviewed Nov 17, 2015
View reviewed changes

benthemonkey added 14 commits November 17, 2015 13:58

🔍 Add class-name-format rule and accompanying tests

de3d663

Add 'allow-leading-underscore' option

73c605a

Add rule to default config

6ec5b69

Update helper isHyphenatedLowercase so special characters are not all…

b40a562

…owed

✅ Add unit tests for helper functions

19a955f

Remove unused test file

45aaf0d

First attempt at collecting suffix extensions to selectors

2c295c6

Update rule and tests to handle suffix extensions

7d44d2f

Remove comment that won't be accurate

57d82ec

Replace 'variable' with 'class'

6da8227

Add jsdocs

b294da8

Add block comments to tests, some cleanup

4be5460

Add tests for attemptTraversal

25d910c

Add nested hyphenatedbem test

fd9235d

benthemonkey force-pushed the feature/class-name-format branch from 3ba99ab to fd9235d Compare November 17, 2015 19:58

DanPurdy added a commit that referenced this pull request Nov 19, 2015

Merge pull request #327 from benthemonkey/feature/class-name-format

d65f6e4

Add rule: class-name-format

DanPurdy merged commit d65f6e4 into sasstools:develop Nov 19, 2015

This was referenced Nov 19, 2015

Adopt "Standard" format? #417

Closed

Add ESLint rule to cover our style of var declarations #419

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rule: class-name-format #327

Add rule: class-name-format #327

benthemonkey commented Oct 18, 2015

DanPurdy commented Oct 18, 2015

bgriffith commented Oct 18, 2015

Snugug commented Oct 18, 2015

BPScott commented Oct 18, 2015

Snugug commented Oct 18, 2015

bgriffith commented Oct 18, 2015

benthemonkey commented Oct 18, 2015

DanPurdy commented Oct 18, 2015

benthemonkey commented Oct 18, 2015

benthemonkey commented Oct 18, 2015

BPScott commented Oct 18, 2015

DanPurdy commented Oct 18, 2015

benthemonkey commented Oct 24, 2015

DanPurdy commented Oct 24, 2015

benthemonkey commented Oct 31, 2015

benthemonkey commented Nov 9, 2015

DanPurdy Nov 11, 2015

DanPurdy commented Nov 11, 2015

benthemonkey commented Nov 11, 2015

DanPurdy commented Nov 11, 2015

benthemonkey commented Nov 11, 2015

benthemonkey commented Nov 12, 2015

DanPurdy Nov 17, 2015

benthemonkey commented Nov 17, 2015

DanPurdy commented Nov 19, 2015

Add rule: class-name-format #327

Add rule: class-name-format #327

Conversation

benthemonkey commented Oct 18, 2015

DanPurdy commented Oct 18, 2015

bgriffith commented Oct 18, 2015

Snugug commented Oct 18, 2015

BPScott commented Oct 18, 2015

Snugug commented Oct 18, 2015

bgriffith commented Oct 18, 2015

benthemonkey commented Oct 18, 2015

DanPurdy commented Oct 18, 2015

benthemonkey commented Oct 18, 2015

Convention Examples

benthemonkey commented Oct 18, 2015

BPScott commented Oct 18, 2015

DanPurdy commented Oct 18, 2015

benthemonkey commented Oct 24, 2015

DanPurdy commented Oct 24, 2015

benthemonkey commented Oct 31, 2015

benthemonkey commented Nov 9, 2015

DanPurdy Nov 11, 2015

Choose a reason for hiding this comment

DanPurdy commented Nov 11, 2015

benthemonkey commented Nov 11, 2015

DanPurdy commented Nov 11, 2015

benthemonkey commented Nov 11, 2015

benthemonkey commented Nov 12, 2015

DanPurdy Nov 17, 2015

Choose a reason for hiding this comment

benthemonkey commented Nov 17, 2015

DanPurdy commented Nov 19, 2015