search

googlearchive / polyclean

BSD 3-Clause "New" or "Revised" License
20 stars 7 forks source link

Fix trimming of spaces #9

Closed rubenstolk closed 8 years ago

rubenstolk commented 8 years ago

Removing all occurrences of 2 spaces is dangerous. For example look at the following definition used in https://github.com/polymerelements/paper-styles:

--shadow-elevation-16dp: {
  box-shadow:
    0 16px 24px 2px rgba(0, 0, 0, 0.14),
    0  6px 30px 5px rgba(0, 0, 0, 0.12),
    0  8px 10px -5px rgba(0, 0, 0, 0.4);
};

For indentation reasons the authors have used two spaces between 0 and 6px/8px. Replacing two spaces or more with nothing would result in 06px / 08px.

This pull requests solves this by reducing two or more spaces to one (two or more will always have the same meaning as one) and by removing leading and trailing spaces afterwards.

rubenstolk commented 8 years ago

Oops, just noticed that there was already an issue for this, #8. Well anyways, I think this is a really good shot at solving that issue...

ebidel commented 8 years ago

LGTM

samccone commented 8 years ago

@rubenstolk I know there are no tests yet.... but it would be SO cool if you added a simple unit test for this :)

rubenstolk commented 8 years ago

@samccone I guess I would leave that to @azakus or someone else to set it up, I don't mind writing the test itself but not very sure what framework and all we're looking at...

samccone commented 8 years ago

Seems like @azakus is OK with mocha :coffee:

https://github.com/PolymerLabs/crisper/tree/master/test

ebidel commented 8 years ago

We won't make you setup tests for the repo in this PR :) I'll let @azakus have the final LGTM and merge b/c I can't update the npm module.

samccone commented 8 years ago

:+1: :+1: just a thought :cloud: obviously not required at all :wink: --- thanks again for this @rubenstolk

dfreedm commented 8 years ago

Yeah, I've been bad by not adding tests :(. LGTM!

rubenstolk commented 8 years ago

Awesome!

rubenstolk commented 8 years ago

Would you make a 1.2.1 on npm?

dfreedm commented 8 years ago

Done!

rubenstolk commented 8 years ago

Cheers and beers on me!

purdrew commented 8 years ago

I believe this fix is breaking cases where css selectors contain ::content preceded by a space. For example, paper-drawer-panel has a selector like this:

iron-selector:not(.narrow-layout) #main ::content [paper-drawer-toggle]

The new code is removing the space between #main and ::content which is significant:

iron-selector:not(.narrow-layout) #main::content [paper-drawer-toggle]
rubenstolk commented 8 years ago

Hmm right! I guess we might want to remove the : from the regex...

rubenstolk commented 8 years ago

Or look for :[^:] patterns, I'll check it out tomorrow.

rubenstolk commented 8 years ago

Sent a new PR #10