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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 4, 2022
Merged

Avoid use of inspect.signature on CommandContribution class #146

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d8c5f2
use fields instead of inspect
tlambert03 Apr 3, 2022
8406043
Merge branch 'main' into fix-pydantic
tlambert03 Apr 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions npe2/_dynamic_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from __future__ import annotations

import inspect
from typing import (
Any,
Callable,
Expand Down Expand Up @@ -31,7 +30,7 @@
C = TypeVar("C", bound=BaseModel)
T = TypeVar("T", bound=Callable[..., Any])

COMMAND_PARAMS = inspect.signature(CommandContribution).parameters

# a mapping of contribution type to string name in the ContributionPoints
# e.g. {ReaderContribution: 'readers'}
CONTRIB_NAMES = {v.type_: k for k, v in ContributionPoints.__fields__.items()}
Expand Down Expand Up @@ -206,7 +205,11 @@ def _store_command(self, func: T, kwargs: dict) -> dict:
"""Create a new command contribution for `func`"""
kwargs.setdefault("title", func.__name__)
kwargs.setdefault("id", f"{self.plugin.manifest.name}.{func.__name__}")
cmd_kwargs = {k: kwargs.pop(k) for k in list(kwargs) if k in COMMAND_PARAMS}
cmd_kwargs = {
k: kwargs.pop(k)
for k in list(kwargs)
if k in CommandContribution.__fields__
}
Comment on lines +208 to +212
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Collaborator

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

not union but intersection

Copy link
Collaborator

@Czaki Czaki Apr 4, 2022 •
edited
Loading

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.

Copy link
Collaborator

@Carreau Carreau Apr 4, 2022 •
edited
Loading

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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'}

cmd = CommandContribution(**cmd_kwargs)
self.commands.append(cmd)
self.plugin.plugin_manager.commands.register(cmd.id, func)
Expand Down