Add typing and docs for rbtools API transport and request.

Review Request #12653 — Created Sept. 29, 2022 and submitted

david
RBTools
release-4.x
rbtools

This change updates the bulk of the rest of the API layer in RBTools
with modern documentation and type hints.

  • Ran unit tests.
  • Posted some changes.
Summary
Add typing and docs for rbtools API transport and request.
Description From Last Updated

'typing.Callable' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

'http.client.UNAUTHORIZED' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

block comment should start with '# ' Column: 9 Error code: E265

reviewbotreviewbot

line too long (82 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

'typing.Union' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

Could add a Deprecated: section to the doc. This applies to the other methods that are deprecated too.

maubinmaubin

This warning shouldn't be here.

maubinmaubin

This should say "Use CachedHTTPResponse.headers instead.".

maubinmaubin

This should say "Use CachedHTTPResponse.code instead.".

maubinmaubin

This warning shouldn't be here.

maubinmaubin

Could add Args: and Returns: sections.

maubinmaubin

Forgot to add typing here.

maubinmaubin

Should we use some other variable names for type and format since these clash with Python's built in methods? Does …

maubinmaubin

Can we :py:class: this?

chipx86chipx86

We can use :py:attr: here.

chipx86chipx86

We can use :py:attr: here. Same with others.

chipx86chipx86

Properties should go before non-constructor methods. Same below.

chipx86chipx86

One of the more confusing aspects of the new typing stuff is that it's hard to tell what's a class-level …

chipx86chipx86

Won't this always end up returning a RootResource or None? I think we should use Optional[RootResource] here. Same with the …

chipx86chipx86

I think we always guarantee a Resource or None from this method as a return type.

chipx86chipx86

Same as get_path.

chipx86chipx86

We don't actually have a return type for this in Transport or SyncTransport. Maybe we should? Do we want to?

chipx86chipx86

Same as login.

chipx86chipx86

Since we have typed parameters in here, let's use the one-param-per-line form to stay consistent with others like this. Same …

chipx86chipx86

This can probably just be: decoder = DECODER_MAP.get(format, DefaultDecoder)

chipx86chipx86

We should have one param per line here.

chipx86chipx86

tuple[...] will break on Python 3.7/3.8. This needs to use Tuple[...] for now.

chipx86chipx86

We re-define this Union a couple times. Let's make a TypeAlias.

chipx86chipx86

One parameter per line here.

chipx86chipx86

One parameter per line with types.

chipx86chipx86

The description can't be indented here.

chipx86chipx86

RBTools 5.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

Can we just super() here?

chipx86chipx86

I think we can safely use super() here.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

These start at 0.

chipx86chipx86

Can we document this while here?

chipx86chipx86

Seems like a reasonable bug fix, but one that slips through the cracks. While small, can we pull this out …

chipx86chipx86

Missing trailing comman and return type (-> None).

chipx86chipx86

Missing trailing comma. Same on functions below.

chipx86chipx86

Same notes as above with the return types here. We should always expect resources. Same below.

chipx86chipx86

These can easily fit one per line starting on the opening line.

chipx86chipx86

__name__

chipx86chipx86

Missing trailing comma.

chipx86chipx86

We should document these, like our other TypedDicts.

chipx86chipx86

One parameter per line when typed.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

Can you add a Version Added?

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
maubin
  1. 
      
  2. rbtools/api/cache.py (Diff revision 2)
     
     

    Could add a Deprecated: section to the doc. This applies to the other methods that are deprecated too.

  3. rbtools/api/cache.py (Diff revision 2)
     
     
     
     

    This warning shouldn't be here.

  4. rbtools/api/cache.py (Diff revision 2)
     
     

    This should say "Use CachedHTTPResponse.headers instead.".

  5. rbtools/api/cache.py (Diff revision 2)
     
     

    This should say "Use CachedHTTPResponse.code instead.".

  6. rbtools/api/cache.py (Diff revision 2)
     
     
     
     

    This warning shouldn't be here.

  7. rbtools/api/factory.py (Diff revision 2)
     
     

    Could add Args: and Returns: sections.

  8. rbtools/api/transport/__init__.py (Diff revision 2)
     
     
     

    Forgot to add typing here.

  9. rbtools/api/utils.py (Diff revision 2)
     
     
     
     
     
     

    Should we use some other variable names for type and format since these clash with Python's built in methods? Does it matter?

    1. It would be nice to, but although it's annoying, this usage isn't technically incorrect, and changing it would require introducing new names with a deprecation cycle. I feel like the whole of the API backend is in need of a rewrite anyway, so I'd rather not touch that for now.

  10. 
      
david
maubin
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. rbtools/api/cache.py (Diff revision 3)
     
     

    Can we :py:class: this?

  3. rbtools/api/cache.py (Diff revision 3)
     
     

    We can use :py:attr: here.

  4. rbtools/api/cache.py (Diff revision 3)
     
     

    We can use :py:attr: here.

    Same with others.

  5. rbtools/api/cache.py (Diff revision 3)
     
     
     
     
     
     
     
     
     

    Properties should go before non-constructor methods.

    Same below.

  6. rbtools/api/client.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     

    One of the more confusing aspects of the new typing stuff is that it's hard to tell what's a class-level attribute and what's documentation for an instance-level attribute.

    The pattern I've been doing is to put this before the blocks of instance attributes:

    ######################
    # Instance variables #
    ######################
    

    That way it's very clear where something is supposed to go as code changes, reducing the risks of mixing up class and instance variables.

  7. rbtools/api/client.py (Diff revision 3)
     
     

    Won't this always end up returning a RootResource or None? I think we should use Optional[RootResource] here.

    Same with the Returns:

    1. This is the abstract adapter that just returns what the transport returns. It's possible that an async transport could return something like an awaitable Future.

    2. Makes sense. The problem's going to be that this bubbles down to the caller. So the caller can't guarantee any type safety on any API resources retrieved.

      With async/await being a thing now, and with only the one backend and all the assumptions around what it returns today, I think maybe we can just assume we get a resource from these. But I know this all needs a rethink anyway.

    3. OK, I'll change them.

  8. rbtools/api/client.py (Diff revision 3)
     
     
     
     
     
     
     

    I think we always guarantee a Resource or None from this method as a return type.

  9. rbtools/api/client.py (Diff revision 3)
     
     
     
     
     
     
     

    Same as get_path.

  10. rbtools/api/client.py (Diff revision 3)
     
     
     
     
     

    We don't actually have a return type for this in Transport or SyncTransport. Maybe we should? Do we want to?

    1. Again, the way things are currently designed, it's possible that a future transport could behave differently. I don't love this model but it's what we have for now and I didn't want to worry about redesigning it.

  11. rbtools/api/client.py (Diff revision 3)
     
     

    Same as login.

  12. rbtools/api/decode.py (Diff revision 3)
     
     

    Since we have typed parameters in here, let's use the one-param-per-line form to stay consistent with others like this.

    Same with others below.

  13. rbtools/api/decode.py (Diff revision 3)
     
     
     
     
     

    This can probably just be:

    decoder = DECODER_MAP.get(format, DefaultDecoder)
    
  14. rbtools/api/errors.py (Diff revision 3)
     
     

    We should have one param per line here.

  15. rbtools/api/request.py (Diff revision 3)
     
     

    tuple[...] will break on Python 3.7/3.8. This needs to use Tuple[...] for now.

  16. rbtools/api/request.py (Diff revision 3)
     
     

    We re-define this Union a couple times. Let's make a TypeAlias.

  17. rbtools/api/request.py (Diff revision 3)
     
     

    One parameter per line here.

  18. rbtools/api/request.py (Diff revision 3)
     
     

    One parameter per line with types.

  19. rbtools/api/request.py (Diff revision 3)
     
     
     
     

    The description can't be indented here.

  20. rbtools/api/request.py (Diff revision 3)
     
     

    RBTools 5.

  21. rbtools/api/request.py (Diff revision 3)
     
     

    Missing a trailing comma.

  22. rbtools/api/request.py (Diff revision 3)
     
     

    Can we just super() here?

    1. Yep. This is left over from python 2 where URLRequest was an old-style object.

  23. rbtools/api/request.py (Diff revision 3)
     
     

    I think we can safely use super() here.

  24. rbtools/api/request.py (Diff revision 3)
     
     

    Missing a trailing comma.

  25. rbtools/api/request.py (Diff revision 3)
     
     
     
     
     
     

    These start at 0.

  26. rbtools/api/request.py (Diff revision 3)
     
     
     

    Can we document this while here?

  27. rbtools/api/request.py (Diff revision 3)
     
     

    Seems like a reasonable bug fix, but one that slips through the cracks. While small, can we pull this out so we can better track it and help with the release notes?

  28. rbtools/api/request.py (Diff revision 3)
     
     
     

    Missing trailing comman and return type (-> None).

  29. rbtools/api/request.py (Diff revision 3)
     
     

    Maybe worth creating a base class we can refer to? Maybe not in this change, but it seems like we should have one.

  30. rbtools/api/transport/__init__.py (Diff revision 3)
     
     

    Missing trailing comma.

    Same on functions below.

  31. rbtools/api/transport/__init__.py (Diff revision 3)
     
     

    Same notes as above with the return types here. We should always expect resources.

    Same below.

  32. rbtools/api/transport/sync.py (Diff revision 3)
     
     
     
     
     
     

    These can easily fit one per line starting on the opening line.

  33. rbtools/api/transport/sync.py (Diff revision 3)
     
     

    __name__

  34. rbtools/api/transport/sync.py (Diff revision 3)
     
     

    Missing trailing comma.

  35. rbtools/api/utils.py (Diff revision 3)
     
     
     
     
     
     
     
     

    We should document these, like our other TypedDicts.

  36. rbtools/api/utils.py (Diff revision 3)
     
     

    One parameter per line when typed.

  37. 
      
david
chipx86
  1. 
      
  2. rbtools/api/client.py (Diff revision 4)
     
     

    Missing a trailing comma.

  3. rbtools/api/client.py (Diff revision 4)
     
     

    Missing a trailing comma.

  4. rbtools/api/client.py (Diff revision 4)
     
     

    Missing a trailing comma.

  5. rbtools/api/errors.py (Diff revision 4)
     
     

    Missing a trailing comma.

  6. rbtools/api/errors.py (Diff revision 4)
     
     

    Missing a trailing comma.

  7. rbtools/api/utils.py (Diff revision 4)
     
     

    Can you add a Version Added?

  8. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.x (ca31831)
Loading...