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.

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

Split Parser and reorganise package #192

Merged
merged 1 commit into from
Feb 27, 2024
Merged

Conversation

masklinn
Copy link
Contributor

Parser turns out to not really make sense as a superclass / ABC: it really only has one useful method, and because parsers use delegation there's no real way to override the utility methods / shortcuts, so they're only useful on the caller / client side but they constrain the implementor (who has to extend the ABC and then possibly deal with multiple-inheritance shenanigans).

Making the core object just a callable protocol instead makes the implementation somewhat simpler and more flexible (e.g. just a function or HoF can be a "parser"), however the convenient utility methods are important for end users and should not be discounted.

For that, keep a wrapper Parser object which can be wrapped around a "parser" in order to provide the additional convenience (similar to the free functions at the root). Importantly, Parser methods can also be used as free functions by passing a "parser" as self, they are intended to be compatible. It doesn't work super well from the typechecking perspective, but it works fine enough.

Consideration was given to making the free functions at the package root parametric on the parser e.g.

def parse(ua: str, resolver: Optional[Resolver] = None, /) -> ParseResult:
    if resolver is None:
        from . import parser as resolver

    return resolver(ua, Domain.ALL).complete()

but that feels like it would be pretty error prone, in the sense that it would be too easy to forget to pass in the resolver, compared to consistently resolving via a bespoke parser, or just installing a parser globally.

Also move things around a bit:

  • move matcher utility functions out of the core, un-prefix them since we're using __all__ for visibility anyway
  • move eager matchers out of the core, similar to the lazy matchers

Fixes #189

@masklinn masklinn force-pushed the resolvers branch 3 times, most recently from 7b4d3d1 to d523ecb Compare February 27, 2024 19:44
@masklinn masklinn enabled auto-merge (rebase) February 27, 2024 19:44
Parser turns out to not really make sense as a superclass / ABC: it
really only has one useful method, and because parsers use delegation
there's no real way to override the utility methods / shortcuts, so
they're only useful on the caller / client side but they constrain the
implementor (who has to extend the ABC and then possibly deal with
multiple-inheritance shenanigans).

Making the core object just a callable protocol instead makes the
implementation somewhat simpler and more flexible (e.g. just a
function or HoF can be a "parser"), however the convenient utility
methods *are* important for end users and should not be discounted.

For that, keep a wrapper `Parser` object which can be wrapped around a
"parser" in order to provide the additional convenience (similar to
the free functions at the root). Importantly, `Parser` methods can
also be used as free functions by passing a "parser" as `self`, they
are intended to be compatible. It doesn't work super well from the
typechecking perspective, but it works fine enough.

Consideration was given to making the free functions at the package
root parametric on the parser e.g.

    def parse(ua: str, resolver: Optional[Resolver] = None, /) -> ParseResult:
        if resolver is None:
            from . import parser as resolver

        return resolver(ua, Domain.ALL).complete()

but that feels like it would be pretty error prone, in the sense that
it would be too easy to forget to pass in the resolver, compared to
consistently resolving via a bespoke parser, or just installing a
parser globally.

Also move things around a bit:

- move matcher utility functions out of the core, un-prefix them since
  we're using `__all__` for visibility anyway
- move eager matchers out of the core, similar to the lazy matchers

Fixes ua-parser#189
@masklinn masklinn merged commit 9e667cb into ua-parser:master Feb 27, 2024
29 checks passed
@masklinn masklinn deleted the resolvers branch February 27, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does Parser make sense as a super class?
1 participant