Refactor of force nested rules to remove duplication

[bgriffith]

PR removes a lot of duplicated functionality from the three force nested rules (force-pseudo-nesting, force-element-nesting and force-attribute-nesting).

It's decreased the amount of code in each rule by 50% through the creation of 2 new helper functions (isNestable and constructSelector) and a slight refactor of the rule code.

It adds 2 other helper functions (capitalize and camelCaseToHyphens) to assist with improving the formatting of the warning messages from these three rules.
Update: 2 string helper functions to improve the formatting of warning messages have been removed and replaced with underscore.string library.

DCO 1.1 Signed-off-by: Ben Griffith gt11687@gmail.com

[Snugug]

Maybe we consider pulling in underscore.string so we don't need to maintain string manipulation functions on our own. Thoughts?

On Oct 17, 2015, at 2:53 PM, Ben Griffith notifications@github.com wrote:

PR removes a lot of duplicated functionality from the three force nested rules (force-pseudo-nesting, force-element-nesting and force-attribute-nesting).

It's decreased the amount of code in each rule by 50% through the creation of 2 new helper functions (isNestable and constructSelector) and a slight refactor of the rule code.

It adds 2 other helper functions (capitalize and camelCaseToHyphens) to assist with improving the formatting of the warning messages from these three rules.

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

#323

Commit Summary

🎨 Added new helper functions and removed need to duplicate code
Merge remote-tracking branch 'upstream/master' into hotfix/correct-warning-messages
Add isNestable helper rule
✅ Add tests for isNestable helper rule
✅ Add constructSelector helper rule tests
🎨 Remove dependancy on .is for helper function
🎨 Refactor force-... rules by moving duplicate functions to helpers
Update comment param issue
Update test descriptions
✅ Add tests for capitalize & camelCaseToHyphens helper rules
🎨 Refactor helper nodes
File Changes

M lib/helpers.js (131)
M lib/rules/force-attribute-nesting.js (172)
M lib/rules/force-element-nesting.js (172)
M lib/rules/force-pseudo-nesting.js (170)
M tests/helpers.js (534)
Patch Links:

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

[bgriffith]

Sure makes sense. I'll pull it in and replace the capitalise helper function.

[bgriffith]

Ok, by requiring the underscore.string library I've removed both the capitalize & camelCaseToHyphens helper functions.

[benthemonkey]

Is there any particular reason you went with Underscore over Lodash? Just curious. I guess @Snugug would be the one to ask. I like that Lodash will let us have per-method dependencies so that the footprint of our tool is as small as possible.

[Snugug]

The library is called underscore.string but is not underscore

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

Is there any particular reason you went with Underscore over Lodash? Just curious.

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

[benthemonkey]

Sorry, thanks for clarifying. But I think my argument still holds. I see that underscore.string has functions that lodash/string doesn't, but do you anticipate needing any of those extra functions? If not, I'm trying to suggest that we can exchange dependency on the entire underscore.string and instead add specifically lodash.capitalize, lodash.kebabcase, etc as needed.

[DanPurdy]

I agree, I was looking through and we're pulling in a lot of functions in that dependency that we don't need or use. Being able to pull in only the functions we require as a dependency would be favourable over a whole library.

[bgriffith]

Completely open on the which library front and happy to pull in individual functions with require("underscore.string/capitalize"); etc

[DanPurdy]

Individual functions would be better than the whole library each time I would say.

[bgriffith]

Done!

[benthemonkey]

I don't mean to be too picky here, as which library we use isn't an important area to devote our time, IMO. But to clarify what I meant about Lodash function-specific dependency:

There is a separate NPM module for each of Lodash's functions, so the project itself only depends on the functions within Lodash that you use.

So we'd change package.json from having underscore.string as a dependency, and instead have lodash.capitalize and lodash.kebabcase

If you look carefully at the dependencies of many npm packages, you'll see fewer instances of dependency on Lodash and more on specific Lodash functions.

Just wanted to clarify. No need to spend time changing things.

[DanPurdy]

Don't worry @benthemonkey I knew what you meant 😉

[bgriffith]

Thanks for clarifying (and sticking with me 😛) @benthemonkey I see a clear advantage now so happy to go with your approach if everyone's in agreement.

[DanPurdy]

Update me!