search

open-i18n / rust-unic

UNIC: Unicode and Internationalization Crates for Rust
https://crates.io/crates/unic
Other
234 stars 24 forks source link

Implement Unicode Name - NR2 #196

Closed eyeplum closed 6 years ago

eyeplum commented 6 years ago

This is a work in progress for implementing Unicode Name - NR2 (#171).

Feedback is welcome!

eyeplum commented 6 years ago

Thanks for the review! I will update according to the comments and add some tests, also will put off NR1 for now. I already have some rough ideas about how NR1 could be implemented, so hopefully a new PR about that will come soon :) Thanks again.

eyeplum commented 6 years ago

Thanks for the review @behnam! I have addressed all the comments and rebased to the latest master. I have also run cargo fmt and committed the changes. Although I'm not able to get cargo clippy to work - tried to install with cargo +nightly install clippy, got compile errors :(

eyeplum commented 6 years ago

Thanks @behnam for the advice. A macro indeed makes the tests look better and easier to change. I have also squashed a few commits.

CAD97 commented 6 years ago

(delegate means you have permission to bors: r+ it now, when you're ready, if I spell it right)

CAD97 commented 6 years ago

bors: delegate+

bors[bot] commented 6 years ago

:v: eyeplum can now approve this pull request

eyeplum commented 6 years ago

@CAD97 sure, I will explore and see. One other thing I'm thinking about is: since len() fmt() and cmp() are all dependent on the string value of self, is it also possible to simply create a string_value() method and call it from those methods? Not sure about the memory footprint though. I notice the initial implementation was going this way but 1a50398 changed it.

CAD97 commented 6 years ago

The trick is to avoid creating the String if unnecessary. Because Display gives ToString for free, the use-site can create a String that way if they want it.

Also note that this crate is #[no_std]. We want to keep these libraries available to no-std environments as much as possible.

eyeplum commented 6 years ago

Ah, make sense. Thanks!

eyeplum commented 6 years ago

@CAD97 Tried with enum Name { NR1, NR2, NR3 }, it does simplify things a lot! So let's do it this way! Thanks for the advice!

eyeplum commented 6 years ago

bors: r+

bors[bot] commented 6 years ago

Build succeeded