Added Rules to detect duplicate selectors & rules #71

Closed
wants to merge 9 commits into
from

Projects

None yet

3 participants

@hpbuniat
Contributor

Maintainability often suffers from copy & paste. The added rules are detecting such issues.
I also added a test-runner to test the current src w/o building.

@nzakas
Contributor
nzakas commented Jun 18, 2011

Thanks for the pull request. I'd like to merge in your rule, but would like to leave the local testrunner out. I stayed away from that because it becomes a maintenance issue (everyone needs to remember to update that file). If you can remove those changes, I'll merge in the rule.

@hpbuniat
Contributor

Thanks for your feedback, i've removed that file.
I'd like to leave the extraction of the testrunner.js in place, to keep the script working for myself.

@nzakas
Contributor
nzakas commented Jun 19, 2011

This makes it a bit trickier to merge. I'd recommend keeping the files you want to use for testing outside of the repo so that you won't accidentally check them in.

@nzakas
Contributor
nzakas commented Jun 22, 2011

Thanks! I'll need to merge this by hand, so I'll close the pull request manually when complete.

@nzakas
Contributor
nzakas commented Jun 22, 2011

In going through this, there are some edge cases that I think are incorrect.

For duplicate selectors, this is perfectly valid:

h1, h2, h3 { font-family: Verdana; }
h1 { font-size: 200% }

Even though the selector is in there twice, there really isn't any other way to do it, so the duplicate-selectors rule in its current state seems like it could come up with a lot of false positives.

For duplicate-rules, I think your intention is if there are two rules with exactly the same properties, they should be combined. However, the following still causes an error:

.class { color: red; text-decoration: underline; }
.foo { color: red; text-decoration: underline; background: red;}

I would expect this to not be a warning because the rules are actually different.

If you'd like to make some changes to handle these cases, I can take another look.

@hpbuniat
Contributor

You're right - this is equal to #80. The rule should mainly address more complex selectors (e.g. div .class .class). Maybe pure tag-selectors can be ignored or reduced to notice-level. I'll try this with some legacy css, which causes my headaches and led to this rule.

@hpbuniat
Contributor

I made some changes to bypass the h1/.. warnings. Unfortunately i was not able to reproduce your warning, but i added your case as test. Did you ran the tests with the current HEAD of the master-branch? Maybe some changes in the parser are leading to this false positive. If so, i will re-create this pull-request from the current master.

@nzakas
Contributor
nzakas commented Jun 28, 2011

Yes, I always use HEAD as the test base. I think we need to take a step back and explore what are the problematic patterns you're really looking to detect. I have doubts that these rules will result in more true positives than false positives, and seeing the patterns you're concerned about would help.

@hpbuniat
Contributor
hpbuniat commented Jul 7, 2011

Ok. I've created a new branch, based on the current HEAD and added the new rules with tests. There are no problems - see: hpbuniat/csslint@cb4bcb7.

Examples are e.g.
duplicate-selectors:
For my understanding, properties should not be added to a rule by accessing the selector twice (might be appropriate for simple selectors). Maybe this might be only a notice, as it is really hard to achieve.

duplicate-rules: http://static.jquery.com/ui/css/demo-docs-theme/ui.theme.css
If there are identical rules, selectors may be combined like selector1, selector2 { .. } or merged to a class (which is of course not always applicable, when it comes to specificity).

@Chris2011

Would be a nice feature. I know the problem by copy and paste and its better to see a warning for this:

body
{
color: #000;
}

p
{
color: #000;
margin-left: 2em;
}

Warning: duplicate code found, solution:

body, p
{
color: #000;
}

p
{
margin-left: 2em;
}

@nzakas
Contributor
nzakas commented Mar 7, 2012

Unfortunately, we were unable to make this work in such a way that everyone agreed the problems were problems.

@nzakas nzakas closed this Mar 7, 2012
@Chris2011

I know its long time ago closed but what is only to show the error? So only see the warning/error: "propertyname - valuename" should only be defined once. And show the lines where are the property - value pair.

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