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 FromStr trait for Key #4

Merged
merged 5 commits into from
Mar 4, 2020
Merged

Add FromStr trait for Key #4

merged 5 commits into from
Mar 4, 2020

Conversation

farmaazon
Copy link
Contributor

Extended the convert.py script, so it implement trait FromStr for Key. Thanks to that, one can now parse strings from e.g. js_sys::KeyboardEvent::key() and have the enum value.

Copy link
Owner

@pyfisch pyfisch left a comment

Choose a reason for hiding this comment

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

Looks good!

Can you bump the minimum supported Rust version so Travis does not fail?

convert.py Show resolved Hide resolved
src/key.rs Outdated
fn from_str(s: &str) -> Result<Self, Self::Err> {
use Key::*;
match s {
ch if ch.chars().take(2).count() == 1 => Ok(Character(ch.to_string())),
Copy link
Owner

Choose a reason for hiding this comment

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

A key string can also consist of a base character and multiple combining characters.
https://w3c.github.io/uievents-key/#keys-unicode
Not sure if it is worth supporting.

Additionally Control Characters should be excluded.

Copy link
Contributor Author

@farmaazon farmaazon Feb 13, 2020

Choose a reason for hiding this comment

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

I could add the exact check if string is a key string, but I'm not sure how do you feel with adding new dependency (I would use https://docs.rs/unic-ucd-category/0.9.0/unic_ucd_category/enum.GeneralCategory.html )

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, the dependency would be quite big for such a small feature.

How about checking that:

  1. there are no control characters in the string (char::is_control)
  2. No characters besides the first are in the ASCII range (char::is_ascii)

This prevents common mistakes like mistyped keynames (e.g. Ennter) from being recognized as characters while allowing all characters specified. If the need arises we can add the dependency later.

@farmaazon farmaazon requested a review from pyfisch March 4, 2020 11:34
@pyfisch pyfisch merged commit 9310355 into pyfisch:master Mar 4, 2020
@pyfisch pyfisch mentioned this pull request Mar 4, 2020
@pyfisch
Copy link
Owner

pyfisch commented Mar 4, 2020

Released as v0.5.0

@stephanemagnenat stephanemagnenat mentioned this pull request Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants