Skip to content

Consider making proper use of / positional arguments #3228

@bluenote10

Description

@bluenote10

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_usage we have a small bug here if field takes 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 that field could 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions