Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NFC support to String.normalize/2 #3805

Merged
merged 1 commit into from
Oct 4, 2015

Conversation

bryanenders
Copy link
Contributor

No description provided.

@bryanenders bryanenders reopened this Sep 29, 2015
@bryanenders bryanenders force-pushed the add-string-normalize-nfc branch 4 times, most recently from 1e29ba4 to d46fd78 Compare September 29, 2015 16:18
@@ -482,4 +496,50 @@ defmodule String.Normalizer do
end

defp combining_class(_), do: 0

def nfd_to_nfc("", acc), do: acc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should those functions be private. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! The irony is that I added syntax support for distinguishing private functions from public ones in Sublime Text, but I never bothered to utilize that distinction in my color scheme. 😆

@josevalim
Copy link
Member

@endersstocker curiosity, can you tell me how big is the String.Normalizer.beam in KB after this PR?

@bryanenders
Copy link
Contributor Author

168 KiB / 172 KB

@bryanenders bryanenders closed this Oct 1, 2015
@bryanenders
Copy link
Contributor Author

@josevalim I’ve optimized it to normalize NFC in a single pass.

@bryanenders bryanenders reopened this Oct 1, 2015
@bryanenders bryanenders force-pushed the add-string-normalize-nfc branch 5 times, most recently from da81896 to 70c65f9 Compare October 1, 2015 02:28
part = :binary.part(binary, 0, n)
case n do
1 -> normalize_nfc(rest, acc <> part)
_ -> normalize_nfc(rest, acc <> compose(normalize(part, :nfd)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's call normalize_nfd(..., "")

@josevalim
Copy link
Member

This is great. I have added a tiny comment. Can you please address it and squashh your changes? ❤️

@bryanenders bryanenders force-pushed the add-string-normalize-nfc branch 2 times, most recently from c11e090 to 4a42861 Compare October 3, 2015 15:06
@bryanenders
Copy link
Contributor Author

It’s good to go.

josevalim added a commit that referenced this pull request Oct 4, 2015
@josevalim josevalim merged commit f6b2ddd into elixir-lang:master Oct 4, 2015
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@bryanenders bryanenders deleted the add-string-normalize-nfc branch October 5, 2015 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants