-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
ucd-generate: add joining-type sub-command #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all LGTM with one minor nit, unless I'm mistaken!
src/joining_type.rs
Outdated
Error::Other(format!( | ||
"Unable to find general category '{}'", | ||
name | ||
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be inclined to panic here, no? That is, this error should never be reached. The only way it could be reached, I think, is with a programmer error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're right. An invalid value needs to make it though canonicalization successfully too before hitting this branch. I've changed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more nit!
src/joining_type.rs
Outdated
use ucd_parse::{self, ArabicShaping}; | ||
|
||
use args::ArgMatches; | ||
use error::{Error, Result}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there is an unused import here?
Also, I would just squash this back into the original commit. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh dang it, sorry about that. Fixed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, thanks!
This PR is on crates.io in |
This PR makes use of the ability to parse
ArabicShaping.txt
to implement joining type table generation. To do this it also needs to do general category lookups, so I extracted aexpand_into_categories
method from thegeneral_category
command in order for it to be able to do this.Sample output: