-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Consider making proper use of / positional arguments #3228
Copy link
Copy link
Closed
Description
Currently elasticsearch.dsl.query uses a sub-optimal approach for handling positional-only arguments: It prefixes the argument names by underscores to "hide" them, but doesn't use the / operator to make them properly positional-only.
This has a few downsides. Consider this simplified example (example on mypy playground to demonstrate typing issue):
from typing import Any
# Just mimicking what elasticsearch.dsl.query does:
DEFAULT = "DEFAULT_SENTINEL"
class Query:
...
class Range(Query):
# This replicates how things are currently implemented, no `/` is used.
def __init__(
self,
_field: str = DEFAULT,
_value: str = DEFAULT,
**kwargs: Any,
):
if _field is not DEFAULT:
kwargs[str(_field)] = _value
super().__init__(**kwargs)
class RangeImproved(Query):
# This would be an improved version.
def __init__(
self,
_field: str = DEFAULT,
_value: str = DEFAULT,
/, # positional arg separator avoids mixing kwargs with positonal args.
**kwargs: Any,
):
if _field is not DEFAULT:
kwargs[str(_field)] = _value
super().__init__(**kwargs)
def example_usage(field: str):
query1 = Range(**{field: {"qt": 42}})
query2 = RangeImproved(**{field: {"qt": 42}}) Issues with the original implementation lacking the / positional argument separator:
- Technically in the
example_usagewe have a small bug here iffieldtakes the values"_field"or"_value", because they would actually be turned into the corresponding "hidden" positional arguments. This would break things at runtime, because the forwarding logic would no longer work this way. - Type checkers correctly notice this issue: There will be a type error in the
Range(**{field: {"qt": 42}})expression, because the type checker considers the possibility thatfieldcould be turned into these positional arguments (by accident). Users would have to suppress these type errors, or try to work around them with complex type guards to rule out the possibility that the kwargs match with these pseudo-hidden positional argument names.
Using / would fix these issues, because it is no longer possible to pass a kwargs as a positional argument by accident. Note that there is no type error any more in the query2 line.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels
Type
Fields
Give feedbackNo fields configured for issues without a type.