Does Parser make sense as a super class? #189
Milestone
Comments
|
python/mypy#8235 (comment) for a way to test protocol conformance relatively easily |
masklinn
added a commit
to masklinn/uap-python
that referenced
this issue
Feb 27, 2024
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
added a commit
to masklinn/uap-python
that referenced
this issue
Feb 27, 2024
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
added a commit
to masklinn/uap-python
that referenced
this issue
Feb 27, 2024
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
added a commit
to masklinn/uap-python
that referenced
this issue
Feb 27, 2024
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
added a commit
to masklinn/uap-python
that referenced
this issue
Feb 27, 2024
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
added a commit
that referenced
this issue
Feb 27, 2024
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Need to go back to the original consideration about them: cross-parser delegation can only happen with
__call__, theparse*helpers can't be overridden, because each parser layer will immediately call into its own__call__, there is noparse*calling through the chain.As such, it may not make that much sense for
Parserto be an abstract class at the top of the hierarchy? Maybe each parser should just be a callable (with the current__call__method), and the "parser" interface would be a top-level helper / wrapper around a pile of whatever the callables are (resolver?)This also means the simpler thingies can be straight up functions e.g. the noop parser from the hitrates script (the counter probably still needs to be a class in order to hold state).
On average it doesn't actually change anything, save removing the inheritance, because almost every parser has state, that state could be reified as a closure instead but I'm not entirely sure it's valuable... though maybe it is? Check out how it works.
edit: trying it out, it does make parsers (resolvers?) a bit simpler to implement, but instantiating one kinda feels bad as you need to wrap the same object around the stack of resolvers every time.
Maybe the wrapper is not necessary though? Instead the top-level utility functions can do the job and take an optional parser? On the other hand that seems a bit dubious from a reliability perspective, it seems way too easy to call
ua_parser.parse(s)when you wantedua_parser.parse(s, myparser). And removing the easy interface or requiring explicitly passing aNoneparser (or w/e) is pretty awful.The text was updated successfully, but these errors were encountered: