- Notifications
You must be signed in to change notification settings - Fork10
Add function to parse script from string#7
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
scripts/unicode.py Outdated
| #[inline] | ||
| pub(crate) fn inner_from_short_name(input: &str) -> Option<Self> { | ||
| match input { | ||
| "" => Some(Script::Unknown), |
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.
should this branch exist? Perhaps we should not return an Option and use Unknown as the catch all
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 thinkOption may be helpful, because if some folk will accidentally usefrom_short_name instead offrom_full_namethey'll get aNone, which is easier to find early.
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.
However, I'm not insisting, if you think that it's better to not useOption here, I'll be happy to change the code.
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'm okay returning an Option but we shouldn't have the empty branch imo
According to discussion inrust-lang/rust-clippy#7400
r?@Manishearth