refactor: Add util function for checking kwarg count #1423
Conversation
github-actions
bot
added
t-tooling
| specified_params = sum(1 for param in [id, name, alias] if param is not None) | ||
| if specified_params > 1: | ||
| raise ValueError('Only one of "id", "name", or "alias" can be specified, not multiple.') | ||
| limit_kwarg_count(alias=alias, name=name, id=id) |
There was a problem hiding this comment.
I'm not sure about the name, limit_kwarg_count doesn't really suggest that this line will throw if there's more than one non-None value passed to the function. Also having to repeat each kwarg twice is pretty unwieldy.
There was a problem hiding this comment.
Using more explicit name.
Also having to repeat each kwarg twice is pretty unwieldy.
That is to have the names of the arguments available in the raised error. Is there a more clever way?
There was a problem hiding this comment.
Is there a more clever way?
I don't think so, unless you want to do some real voodoo (like stack frame inspection).
There was a problem hiding this comment.
In that case, let's keep it this way for now. This is clear and readable.
Pijukatel
force-pushed
the
limit-kwarg-count
branch
from
3591a19 to
c4567d6
Compare
Pijukatel
force-pushed
the
limit-kwarg-count
branch
from
c4567d6 to
d59cf31
Compare
| """Raise ValueError if there are more kwargs then max_kwargs.""" | ||
| if len(kwargs) - Counter(kwargs.values())[None] > max_kwargs: | ||
| kwargs_names = [f'"{kwarg_name}"' for kwarg_name in kwargs] | ||
| raise ValueError(f'Only one of {", ".join(kwargs_names)} can be specified, not multiple.') |
There was a problem hiding this comment.
Can you also tell the user which ones were specified?
There was a problem hiding this comment.
Ok, added to the exception text
Pijukatel
force-pushed
the
limit-kwarg-count
branch
from
d2d67a5 to
70477c9
Compare
| if len(kwargs) - Counter(kwargs.values())[None] > max_kwargs: | ||
| kwargs_names = [f'"{kwarg_name}"' for kwarg_name in kwargs] | ||
| used_kwargs_names = [f'"{kwarg_name}"' for kwarg_name, value in kwargs.items() if value is not None] |
There was a problem hiding this comment.
Since we need to use list comprehension for used_kwargs_names anyway, do we really need Counter?
maybe just
used_kwargs_names = [f'"{kwarg_name}"' for kwarg_name, value in kwargs.items() if value is not None]
if len(used_kwargs_names ) > max_kwargs:
Description
Testing