Improve String#squish whitespaces matching #8830

Merged
merged 1 commit into from Jan 28, 2013

Conversation

Projects
None yet
3 participants
Contributor

antoinelyset commented Jan 8, 2013

Hi,

My first PR :).
This is a simple improvement of the regex detecting the whitespaces in String#squish.
The actual regex is an "ASCII version" and my PR replaces it with a more UTF-8 compliant.

More infos on UTF-8 whitespaces : http://en.wikipedia.org/wiki/Unicode_character_property#Whitespace

Thanks for your contribution :)

Owner

fxn commented Jan 14, 2013

It is fine for 4.0 in my opinion.

A couple of remarks:

  • I believe it should also strip [:space:].
  • The API and the AS guide should document this detail.
  • We need a CHANGELOG entry in Active Support, please follow the existing style.
Contributor

antoinelyset commented Jan 20, 2013

Thanks. I will try to do my best.

Contributor

antoinelyset commented Jan 22, 2013

I updated the PR. Is it ok now ?

Owner

fxn commented Jan 22, 2013

I think it would be better to say that you improve it to handle Unicode whitespace, rather than UTF8 whitespace. Because being whitespace is an abstract property defined by Unicode, it is not specific to an encoding.

If you edit that I think it is good to go.

Contributor

antoinelyset commented Jan 22, 2013

You're totally right. I did the modification. 🆙

fxn added a commit that referenced this pull request Jan 28, 2013

Merge pull request #8830 from antoinelyset/master
Improve String#squish whitespaces matching

@fxn fxn merged commit 30034df into rails:master Jan 28, 2013

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