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

Keybinding comparing itself to str is really weird. #108

Closed
Carreau opened this issue Jun 23, 2023 · 3 comments
Closed

Keybinding comparing itself to str is really weird. #108

Carreau opened this issue Jun 23, 2023 · 3 comments

Comments

@Carreau
Copy link

Carreau commented Jun 23, 2023

  • app-model version: 0.1.4
  • Python version: 3.10
  • Operating System: Mac

Description

I see what you are doing with Keybinding, and trying to make it magically work with strings, but I think it's a footgun in the long run.

What I Did

In [1]: from app_model.types import KeyBinding

In [2]: X = [KeyBinding.from_str('A')]

In [3]: [isinstance(s, str) for s in X]
Out[3]: [False]

In [4]: 'A' in X
Out[4]: True

For me Out[3] logically implies that In [4] Cannot be True. And equal but hash is different is a footgun as well:

In [7]: d = {}

In [8]: d[X[0]] = 1

In [9]: d['A'] # must be 1 as X[0] == 'A' ?

No KeyError.

I think this is make worse by the fact that __str__ return the keybinding in the string form as now code in napari liberally use the object directly in places where it's implicitely str()'d.

@tlambert03
Copy link
Member

I'm open to changes if you want to open a pr for discussion

@Carreau
Copy link
Author

Carreau commented Jun 26, 2023

I'm open to changes if you want to open a pr for discussion

Thanks, I'll try to thinking how we can try to be stricter, and have a path forward with backward compatibility. I'll see if I can incorporate as well some of the logic we have in napari to display platform specific symbols for shortcuts.

@tlambert03
Copy link
Member

fixed by #146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants