search

zspecza / common-tags

πŸ”– Useful template literal tags for dealing with strings in ES2015+
Other
1.99k stars 61 forks source link

A little better RegExps and refactor #130

Closed jakeweary closed 6 years ago

jakeweary commented 6 years ago

Wanted to improve one little regexp but ended up with a few more changes. :)

codecov-io commented 6 years ago

Codecov Report

Merging #130 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #130   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          23     23           
  Lines         152    151    -1     
=====================================
- Hits          152    151    -1
Impacted Files Coverage Ξ”
...c/stripIndentTransformer/stripIndentTransformer.js 100% <100%> (ΓΈ) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update 62ff633...0d43c46. Read the comment docs.

fatfisz commented 6 years ago

All looks good, but I'm not too sure about "\r" in there. In many places in the code only "\n" is used, so I'd leave it like that for consistency - otherwise a bigger refactor would be in order.

jakeweary commented 6 years ago

A am also unsure about that. I personaly use only \n in my code. But I think it would be a good idea to keep \r's because changing the style of newlines is not what people may expect from stripIndents.

Also, as far as I can tell from the code, \r's were untouched before 900aa00a952d67a5295c387e0759da2448931c51 . Now stripIndents removes them but stripIndent keeps. In fact it removes them in a weird way: sequences of newlines like \r\n\r\n\r\n become \n\n\n but regular strings like foo\r\nbar keep that \r in the end. So stripIndents just mixes styles of newlines, which is not really a good thing.

I think a decent solution would be not to mess with \r's at all and a better (?) one would be removing them only when they're being a part of identation (for some really strange reason, haha) which requires some more complex regexps.

I also consider removing \r's completely and not really against it. At least it won't affect me and my code. But again, we need something better than [^\S\n] since it just does weird things.

What do you think? :)

fatfisz commented 6 years ago

I'd leave [^\S\n] and wait until people complain πŸ˜‰ If push comes to shove I think having a "turn \n into \r\n" tag and a "turn \r\n into \n" tag will be a nice better solution. This, or a big refactor for the whole lib - I think it's better to consistently not work with something than work in some special cases.

Btw. the behavior was not changed in https://github.com/declandewet/common-tags/commit/900aa00a952d67a5295c387e0759da2448931c51, because previously splitting was done by \n, and then '\r'.trimLeft() === '' (at least where String.prototype.trimLeft is implemented πŸ˜„).

jakeweary commented 6 years ago

Ah, now I see. I saw trimLeft and it was taken literally by my brain. πŸ˜„

I thought I'm returning consistensy back with my PR (at least for stripIndents), but in reality I'm doing the opposite. I had to test it more before doing PR, sorry about that.

Well, since my PR doesn't really make sense anymore, should I close it?

fatfisz commented 6 years ago

I really dig early returns instead of reassigning to arguments, so I'd merge that happily :)

jakeweary commented 6 years ago

This PR is definitely more trouble than it's worth now. πŸ˜„

fatfisz commented 6 years ago

Really sorry about this, it was not my intention to drag this out for so long. If you haven't done that refactor, I'd probably do the same thing eventually, but since you put in some work I didn't want to dismiss it.

Let me know here if you want to be added to the list of the contributors; I'll add you in a separate commit then so to not block this PR anymore.