Add typing and docs for rbtools API transport and request.

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

Information

RBTools
release-4.x

Reviewers

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 ID
Add typing and docs for rbtools API transport and request.
This change updates the bulk of the rest of the API layer in RBTools with modern documentation and type hints. Testing Done: - Ran unit tests. - Posted some changes.
2f57e4cb8e157693ba1a1665d1ac47c107acf35a
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)
     
     
    Show all issues

    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)
     
     
     
     
    Show all issues

    This warning shouldn't be here.

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

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

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

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

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

    This warning shouldn't be here.

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

    Could add Args: and Returns: sections.

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

    Forgot to add typing here.

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

    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)
     
     
    Show all issues

    Can we :py:class: this?

  3. rbtools/api/cache.py (Diff revision 3)
     
     
    Show all issues

    We can use :py:attr: here.

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

    We can use :py:attr: here.

    Same with others.

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

    Properties should go before non-constructor methods.

    Same below.

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

    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)
     
     
    Show all issues

    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)
     
     
     
     
     
     
     
    Show all issues

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

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

    Same as get_path.

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

    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)
     
     
    Show all issues

    Same as login.

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

    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)
     
     
     
     
     
    Show all issues

    This can probably just be:

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

    We should have one param per line here.

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

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

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

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

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

    One parameter per line here.

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

    One parameter per line with types.

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

    The description can't be indented here.

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

    RBTools 5.

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

    Missing a trailing comma.

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

    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)
     
     
    Show all issues

    I think we can safely use super() here.

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

    Missing a trailing comma.

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

    These start at 0.

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

    Can we document this while here?

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

    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)
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    Missing trailing comma.

    Same on functions below.

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

    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)
     
     
     
     
     
     
    Show all issues

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

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

    __name__

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

    Missing trailing comma.

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

    We should document these, like our other TypedDicts.

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

    One parameter per line when typed.

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

    Missing a trailing comma.

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

    Missing a trailing comma.

  4. rbtools/api/client.py (Diff revision 4)
     
     
    Show all issues

    Missing a trailing comma.

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

    Missing a trailing comma.

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

    Missing a trailing comma.

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

    Can you add a Version Added?

  8. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.x (ca31831)