-
Notifications
You must be signed in to change notification settings - Fork 28
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
Avoid use of inspect.signature on CommandContribution class #146
Conversation
Codecov Report
@@ Coverage Diff @@
## main #146 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 26
Lines 1719 1717 -2
=========================================
- Hits 1719 1717 -2
Continue to review full report at Codecov.
|
pre-commit failure is unrelated... upstream problem |
Trying closing and reopening for precommit. |
cmd_kwargs = { | ||
k: kwargs.pop(k) | ||
for k in list(kwargs) | ||
if k in CommandContribution.__fields__ | ||
} |
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.
cmd_kwargs = { | |
k: kwargs.pop(k) | |
for k in list(kwargs) | |
if k in CommandContribution.__fields__ | |
} | |
kwargs_set = set(kwargs) | |
cmd_kwargs = { | |
k: kwargs.pop(k) | |
for k in kwargs_set | |
if k in CommandContribution.__fields__ | |
} |
I know that this is only a few positions, but creating a list every time? Some less experienced users may use this as inspiration in places where this condition is not satisfied.
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 mean you don't even need the set/list, as 'k in kwargs' will iterate over the dict keys, and if you use the set, then why not kwargs_set = set(kwargs).union(set(CommandContribution.__fields__))
and avoid the test.
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.
not union but intersection
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, maybe to early. I replace in the head if check with iteration. Ignore this change request.
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.
not union but intersection
Ghhhhaaaa. Pfrrlrlrlflfr. Yes.
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.
... How many programmers do you need to get a dict comprehension right ...
Today is not a functioning brain day.
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 mean you don't even need the set/list, as 'k in kwargs' will iterate over the dict keys,
we do in this case, since kwargs is getting changed on each iteration by kwargs.pop
...
but creating a list every time?
it's not creating a list on each iteration either way (the list would only get called on the first evaluation of the comprehension)
In [13]: class T:
...: def __iter__(self):
...: print('iter')
...: yield from ['a','b','c']
...:
In [14]: kwargs = T()
In [15]: list(kwargs)
iter
Out[15]: ['a', 'b', 'c']
In [16]: {k for k in list(kwargs) if k in ['a','b']}
iter
Out[16]: {'a', 'b'}
In [17]: {k for k in kwargs if k in ['a','b']}
iter
Out[17]: {'a', 'b'}
see #147 |
fixes #144
This fixes the breakage of napari used with PySide2 in npe2=0.2.2, just by "avoiding" using inspect.signature ... this is super annoying bug in PySide2, that affects the ability to modify signatures of any python object. See napari/napari#2265 for details