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

Consider using @typing.overload instead of Union input/output types #32

Open
christhompson opened this issue Jul 10, 2024 · 1 comment

Comments

@christhompson
Copy link

I recently pulled in the new release of the library into my project and had some difficulty with API changes to the privatesuffix() method in particular. My project's existing code that used the old version passed in a str hostname and relied on getting an Optional[str] back, handling the result like this (as an isolated example):

r = self.psl.privatesuffix(hostname)
return r if r else r hostname

The updated version of privatesuffix() now takes in a RelaxDomain (i.e., Union[str, BytesTuple, Iterable]) and returns an Optional[Domain] (i.e., Optional[Union[str, BytesTuple]). Looking over the code, my understanding is that this is extending the method to include a new specialization for additionally taking in a BytesTuple or Iterable, and in turn outputting a BytesTuple, but the "original" str version still exists (that is, if you pass in a str you get out an Optional[str] effectively). This means that our calling code now does:

r = self.psl.privatesuffix(hostname)
return r if isinstance(r, str) else hostname

This isn't too bad, but it does mean we're making some assumptions about the internal implementation of this method, where the API/types contract is a bit opaque (there's no type-system guarantee that str in means str out).

If this was instead implemented as method overloads using @typing.overload, the type system would know that the str in type was explicitly connected to the str out type, like this:

@overload
def privatesuffix(self, domain: str, ...) -> Optional[str]: ...
@overload
def privatesuffix(self, domain: BytesTuple, ...) -> Optional[BytesTuple]: ...
def privatesuffix(self, domain, ...) -> Optional[str] | Optional[BytesTuple]:
    # actual implementation here

If this was something that seemed desirable for this library, I'm happy to try working on a PR, but wanted to discuss before doing that (and if it was desirable I'd want to discuss how far to extend this pattern throughout the library). I also understand if the additional complexity doesn't seem warranted. Ultimately the downstream burden we have for this is pretty minimal :-)

@ko-zu
Copy link
Owner

ko-zu commented Jul 11, 2024

Thank you for your feedback and for pointing out the issue. I agree that it should have explicit typing.

The current expected domain input/output should be:

  • str -> Optional[str]
  • BytesTuple-> Optional[BytesTuple]
  • Interable[ByteString] -> Optional[BytesTuple]

The method definition should be like:

    @overload
    def suffix(self,
               domain: str,
               accept_unknown: Optional[bool] = None,
               *,
               keep_case: bool = False) -> Optional[str]: ...
    @overload
    def suffix(self,
               domain: Union[BytesTuple, Iterable[ByteString]],
               accept_unknown: Optional[bool] = None,
               *,
               keep_case: bool = False) -> Optional[BytesTuple]: ...
    def suffix(self,
               domain: RelaxDomain,
               accept_unknown: Optional[bool] = None,
               *,
               keep_case: bool = False) -> Optional[Domain]:
        """ Alias for privatesuffix """
        return self.privatesuffix(domain, accept_unknown=accept_unknown, keep_case=keep_case)

(Unfortunately, the Union operator was not available in Python 3.5)

I believe this would be sufficient for the return type check and the argument hinting. VS Code renders the second difinition as domain: BytesTuple | Iterable[ByteString].
Please let me know your opinion.

Any PR is welcome, and I really appreciate your contribution. However, if you don't mind, I will implement this in v1.0.2.

ko-zu added a commit that referenced this issue Jul 13, 2024
Add method overload to explicitly declare str-to-str input/output,
as christhompson commented in issue #32.

Signed-off-by: ko-zu <[email protected]>
ko-zu added a commit that referenced this issue Jul 13, 2024
- Use @typing.overload to indicate str-to-str domain input/output. #32.
- Address deprecation of ByteString in python 3.14.

Signed-off-by: ko-zu <[email protected]>
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

No branches or pull requests

2 participants