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

Feature/property count #407

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

DanPurdy
Copy link
Member

Adds the rule property-count which enforces the maximum number of properties per ruleset. This can be configured to be per ruleset or inclusive of nested rulesets.

Warning Message:

Rule set contains {number} {property/properties} more than the specified maximum of {number} {(includes properties in nested rules)}

Includes the options:

  • max-properties
  • include-nested

Closes #406

DCO 1.1 Signed-off-by: Dan Purdy [email protected]

@DanPurdy DanPurdy added this to the 1.4.0 milestone Nov 15, 2015
@benthemonkey
Copy link
Member

I see that any node type that interrupts the block -> ruleset -> block ... cycle stops the code from checking for further-nested properties. This means that properties nested inside media queries, @at-root, etc. are not included in the nested property count. Is this the desired behavior?

@DanPurdy
Copy link
Member Author

With at-root definitely because it's about making sure you're using the cascade correctly and at-root removes that consideration anyway. As for media queries, that's a question really, do we want to handle that?

I ran this implementation as well as scss-lints version and found similar issues with media queries and at-root so id be open to seeing other scenarios we should support.

@benthemonkey
Copy link
Member

Additionally, until we upgrade Gozales, suffix extensions are being included in nested property counts, even though they aren't nested within their parent when compiled. However, suffix extensions are nested within their grandparent. Perhaps this exception doesn't fall within the "spirit" of this rule. What do you think? Example:

.foo {
  .bar {
    . color: green;
    &-baz {
      color: blue;
    }
  }
}

With include-nested: true, .bar's property count ought to be 1, while either .foo's or .foo .bar-baz's property count is 2.

@DanPurdy
Copy link
Member Author

It's not about how many properties are on the rule after compilation though its about how many properties are within each block in the source. When the nested option is on with the above code the nested count should always be 2 and the unnested counts should be 0, 1, 1.

@DanPurdy
Copy link
Member Author

Will update this to work correctly with mixins etc which will be an improvement over the scss lint version

@DanPurdy
Copy link
Member Author

Gonzales scuppers the .sass version of this when using at-root or media queries as it considers atrules to be siblings of the properties they contain rather than parents which it does in .scss... 😢 and atruleb doesn't exist...

Part of me thinks release this for scss, output a warning for sass users and wait for the gonzales update...

@sasstools/sass-lint-contributors any ideas?

so except for the properties in .bar below everything else is on the same level apparently..

screen shot 2015-11-20 at 00 31 22

@DanPurdy DanPurdy removed this from the 1.4.0 milestone Dec 8, 2015
@Snugug
Copy link
Member

Snugug commented Dec 8, 2015

Ugh. GONZALES!

Yah, I'm OK with that for the moment as long as that's documented.

Maybe we should start adding what syntaxes are supported for each rule if it differs from "all"

@DanPurdy DanPurdy mentioned this pull request Dec 19, 2015
@DanPurdy
Copy link
Member Author

I'll be moving forward with this for the latest version of gonzales-pe as I believe the issue I was encountering with the Sass syntax should be fixed!

@DanPurdy
Copy link
Member Author

I'll be reviving this for the new 1.6 update now once I've verified that sass blocks are indented correctly

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

Successfully merging this pull request may close these issues.

3 participants