Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Don't strip escaped whitespace #3

Merged
merged 8 commits into from Nov 27, 2012

Conversation

Projects
None yet
2 participants
Contributor

SLaks commented Nov 27, 2012

Fixes #2.

I haven't tested this, but it should correctly trim the following cases:

  • #\ ‌ > a -> #\ >a
  • #\ ‌ -> #\ ‌
    In other words, it should strip spaces after escaped whitespace.
Owner

fb55 commented Nov 27, 2012

This will lead to another bug: Trailing whitespace will now add a descendant node at the end.

Also, please don't add the error description as a comment. Out of context, it looks weird.

Contributor

SLaks commented Nov 27, 2012

I tried to fix that in the regex (by adding |$ in the group)

Contributor

SLaks commented Nov 27, 2012

Any idea why Travis isn't reporting build status of the PR?

Contributor

SLaks commented Nov 27, 2012

The tests now pass

Contributor

SLaks commented Nov 27, 2012

I added the comment because I like to document regexes. (especially since this one has non-obvious corner cases)

What would you prefer?

@fb55 fb55 and 1 other commented on an outdated diff Nov 27, 2012

@@ -37,7 +38,8 @@ function unescapeCSS(str){
//based on http://mathiasbynens.be/notes/css-escapes
//TODO support short sequences (/\\\d{1,5} /)
return str.replace(re_escapedCss, function(m, s){
- if(isNaN(s)) return s;
+ // isNaN(' ') is false (!)
+ if(isNaN(s) || re_whitespace.test(s)) return s;
@fb55

fb55 Nov 27, 2012

Owner

Use /\D/.test() as a more general check.

@SLaks

SLaks Nov 27, 2012

Contributor

Good idea

@fb55 fb55 commented on an outdated diff Nov 27, 2012

re_nthElement = /^([+\-]?\d*n)?\s*([+\-])?\s*(\d)?$/,
re_escapedCss = /\\(\d{6}|.)/g,
+ re_nonNumeric = /^\D$/,
@fb55

fb55 Nov 27, 2012

Owner

Tabs, please :)

Contributor

SLaks commented Nov 27, 2012

It looks like all of the declarations, except the line I added, were using spaces, whereas most of the rest of the file was using tabs.

I tabified all of both source files.

Owner

fb55 commented Nov 27, 2012

Whee, I ment it the other way: Grouped var declarations should have leading spaces. Smart tabs etc. Sorry.

Contributor

SLaks commented Nov 27, 2012

I've never seen that style before.
I guess you want the variable names to line up regardless of the tab width?

Owner

fb55 commented Nov 27, 2012

Exactly, yes.

That regexp is still pretty easy to understand, so no comments please.

Please combine the commits, and I'll merge this.

Contributor

SLaks commented Nov 27, 2012

You're indenting arrays inconsistently (the tests array uses spaces; all other arrays use tabs)

@fb55 fb55 added a commit that referenced this pull request Nov 27, 2012

@fb55 fb55 Merge pull request #3 from Unroll-Me/master
Don't strip escaped whitespace
d4220b7

@fb55 fb55 merged commit d4220b7 into fb55:master Nov 27, 2012

Owner

fb55 commented Nov 27, 2012

That happens when you use spaces once and your texteditor autocompletes them every time in the future…

Anyway, thanks!

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