Add String.Unicode.{lstrip,rstrip} #683

Merged
merged 1 commit into from Dec 3, 2012

Conversation

Projects
None yet
2 participants
Contributor

devinus commented Nov 30, 2012

No description provided.

Contributor

devinus commented Dec 1, 2012

Don't merge this yet.

Owner

josevalim commented Dec 1, 2012

Ok. In any case, could you please decouple the refactoring of the current code from the actual feature? It'd better to review and to blame the code later.

Contributor

devinus commented Dec 1, 2012

Okay, ready for peer review. @yrashk @josevalim @l4u

Owner

josevalim commented Dec 2, 2012

Ah, this looks great, thanks @devinus!

One thing it is important to distinguish is if String.strip should strip all spaces or only white spaces. In Unicode, we have a couple characters that are spaces but they are not a whitespace. Example:

http://www.fileformat.info/info/unicode/char/a0/index.htm

My initial reaction is that we should strip only whitespaces and therefore we could skip the category check as a whole and rely only on bidi.

Contributor

devinus commented Dec 2, 2012

@josevalim That's how the first patch was. I didn't need to change it. Let me fix that.

josevalim added a commit that referenced this pull request Dec 3, 2012

Merge pull request #683 from devinus/unicode-strip
Add String.Unicode.{lstrip,rstrip}

@josevalim josevalim merged commit fa9a78f into elixir-lang:master Dec 3, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment