aanand / deadweight

NOT MAINTAINED
MIT License
1.19k stars 52 forks source link

Support for CSS selectors Level 3 and 4 #15

Closed tadast closed 12 years ago

tadast commented 12 years ago

Hi Annand,

deadweight breaks when CSS files have selectors like these:

a.button:not(:disabled):hover { ... }
@-webkit-keyframes appear { ... }

This patch fixes it.

aanand commented 12 years ago

Looks nice. Could you perhaps use the /x flag to split the big regex out onto multiple lines and document it line-by-line?

I feel like something that hairy should have some more test cases, too :)

On 17 Feb 2012, at 18:34, Tadas Tamosauskasreply@reply.github.com wrote:

Hi Annand,

deadweight breaks when CSS files have selectors like these:

a.button:not(:disabled):hover { ... } @-webkit-keyframes appear { ... }

This patch fixes it.

You can merge this Pull Request by running:

git pull https://github.com/tadast/deadweight master

Or you can view, comment on it, or merge it online at:

https://github.com/aanand/deadweight/pull/15

-- Commit Summary --

  • Support more CSS3/CSS4 selectors

-- File Changes --

M lib/deadweight.rb (2) M test/deadweight_test.rb (6) M test/fixtures/index.html (1) M test/fixtures/style.css (8)

-- Patch Links --

https://github.com/aanand/deadweight/pull/15.patch https://github.com/aanand/deadweight/pull/15.diff


Reply to this email directly or view it on GitHub: https://github.com/aanand/deadweight/pull/15

tadast commented 12 years ago

I've updated my commit. I agree about hairiness, but I don't want to copy the expression to the comment and document it line by line, because it is not DRY. Also, one does not simply comment in the middle of the multi-line regex... So I just split it into multiple lines.

Regarding tests, I can write some isolated unit tests for the #split method if you want, but it wouldn't follow the current style. I also don't want to clutter the global test setup with different tests for each case, instead I've added a single worst edge case I could think of :)

pboling commented 12 years ago

Does this fix the problem with CSS3 substring matchers support?

aanand commented 12 years ago

@pboling: Doesn't look like it I'm afraid - @tadast's patch only strips the : suffix from selectors.

@tadast: After reading over this again, would there be any danger in simply matching against /:.*/?

tadast commented 12 years ago

@aanand I actually had the same though in the shower and forgot it later on. I can't think of a case where matching against /:.*/ would break anything. Well of course we still need to strip all @-webkit, and @media print, and friends. Should I go for it?

aanand commented 12 years ago

I say yes.

On 23 Feb 2012, at 17:23, Tadas Tamosauskasreply@reply.github.com wrote:

@aanand I actually had the same though in the shower and forgot it later on. I can't think of a case where matching against /:.*/ would break anything. Well of course we still need to strip all @-webkit, and @media print, and friends. Should I go for it?


Reply to this email directly or view it on GitHub: https://github.com/aanand/deadweight/pull/15#issuecomment-4140684