Summary

Add typing and docs for rbtools API transport and request.

Review Request #12653 — Created Sept. 29, 2022 and submitted Oct. 10, 2022, 9:54 p.m.

Information

Owner

david

Repository

RBTools

Branch

release-4.x

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

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.

Commits

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

Issues

Description	From	Last Updated
'typing.Callable' imported but unused Column: 1 Error code: F401	reviewbot	Sept. 29, 2022, 10:17 p.m.
'http.client.UNAUTHORIZED' imported but unused Column: 1 Error code: F401	reviewbot	Sept. 29, 2022, 10:17 p.m.
block comment should start with '# ' Column: 9 Error code: E265	reviewbot	Sept. 29, 2022, 10:17 p.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	Sept. 29, 2022, 10:17 p.m.
'typing.Union' imported but unused Column: 1 Error code: F401	reviewbot	Sept. 29, 2022, 10:17 p.m.
Could add a Deprecated: section to the doc. This applies to the other methods that are deprecated too.	maubin	Oct. 4, 2022, 8:38 a.m.
This warning shouldn't be here.	maubin	Oct. 4, 2022, 8:38 a.m.
This should say "Use CachedHTTPResponse.headers instead.".	maubin	Oct. 4, 2022, 8:38 a.m.
This should say "Use CachedHTTPResponse.code instead.".	maubin	Oct. 4, 2022, 8:38 a.m.
This warning shouldn't be here.	maubin	Oct. 4, 2022, 8:38 a.m.
Could add Args: and Returns: sections.	maubin	Oct. 4, 2022, 8:42 a.m.
Forgot to add typing here.	maubin	Oct. 4, 2022, 8:42 a.m.
Should we use some other variable names for type and format since these clash with Python's built in methods? Does …	maubin	Oct. 4, 2022, 8:44 a.m.
Can we :py:class: this?	chipx86	Oct. 5, 2022, 9 p.m.
We can use :py:attr: here.	chipx86	Oct. 5, 2022, 9:01 p.m.
We can use :py:attr: here. Same with others.	chipx86	Oct. 5, 2022, 9:01 p.m.
Properties should go before non-constructor methods. Same below.	chipx86	Oct. 5, 2022, 9:01 p.m.
One of the more confusing aspects of the new typing stuff is that it's hard to tell what's a class-level …	chipx86	Oct. 5, 2022, 9:02 p.m.
Won't this always end up returning a RootResource or None? I think we should use Optional[RootResource] here. Same with the …	chipx86	Oct. 6, 2022, 4:50 p.m.
I think we always guarantee a Resource or None from this method as a return type.	chipx86	Oct. 5, 2022, 9:04 p.m.
Same as get_path.	chipx86	Oct. 5, 2022, 9:04 p.m.
We don't actually have a return type for this in Transport or SyncTransport. Maybe we should? Do we want to?	chipx86	Oct. 5, 2022, 9:05 p.m.
Same as login.	chipx86	Oct. 5, 2022, 9:05 p.m.
Since we have typed parameters in here, let's use the one-param-per-line form to stay consistent with others like this. Same …	chipx86	Oct. 5, 2022, 9:06 p.m.
This can probably just be: decoder = DECODER_MAP.get(format, DefaultDecoder)	chipx86	Oct. 5, 2022, 9:06 p.m.
We should have one param per line here.	chipx86	Oct. 5, 2022, 9:06 p.m.
tuple[...] will break on Python 3.7/3.8. This needs to use Tuple[...] for now.	chipx86	Oct. 5, 2022, 9:07 p.m.
We re-define this Union a couple times. Let's make a TypeAlias.	chipx86	Oct. 5, 2022, 9:08 p.m.
One parameter per line here.	chipx86	Oct. 5, 2022, 9:08 p.m.
One parameter per line with types.	chipx86	Oct. 5, 2022, 9:08 p.m.
The description can't be indented here.	chipx86	Oct. 5, 2022, 9:09 p.m.
RBTools 5.	chipx86	Oct. 5, 2022, 9:09 p.m.
Missing a trailing comma.	chipx86	Oct. 5, 2022, 9:09 p.m.
Can we just super() here?	chipx86	Oct. 5, 2022, 9:09 p.m.
I think we can safely use super() here.	chipx86	Oct. 5, 2022, 9:10 p.m.
Missing a trailing comma.	chipx86	Oct. 5, 2022, 9:10 p.m.
These start at 0.	chipx86	Oct. 5, 2022, 9:10 p.m.
Can we document this while here?	chipx86	Oct. 5, 2022, 9:12 p.m.
Seems like a reasonable bug fix, but one that slips through the cracks. While small, can we pull this out …	chipx86	Oct. 6, 2022, 10:10 a.m.
Missing trailing comman and return type (-> None).	chipx86	Oct. 5, 2022, 9:13 p.m.
Missing trailing comma. Same on functions below.	chipx86	Oct. 5, 2022, 9:13 p.m.
Same notes as above with the return types here. We should always expect resources. Same below.	chipx86	Oct. 5, 2022, 9:13 p.m.
These can easily fit one per line starting on the opening line.	chipx86	Oct. 5, 2022, 9:14 p.m.
__name__	chipx86	Oct. 5, 2022, 9:14 p.m.
Missing trailing comma.	chipx86	Oct. 5, 2022, 9:14 p.m.
We should document these, like our other TypedDicts.	chipx86	Oct. 6, 2022, 10:06 a.m.
One parameter per line when typed.	chipx86	Oct. 5, 2022, 9:16 p.m.
Missing a trailing comma.	chipx86	Oct. 6, 2022, 4:51 p.m.
Missing a trailing comma.	chipx86	Oct. 6, 2022, 4:51 p.m.
Missing a trailing comma.	chipx86	Oct. 6, 2022, 4:51 p.m.
Missing a trailing comma.	chipx86	Oct. 6, 2022, 4:51 p.m.
Missing a trailing comma.	chipx86	Oct. 6, 2022, 4:51 p.m.
Can you add a Version Added?	chipx86	Oct. 6, 2022, 4:52 p.m.

flake8 failed.

JSHint passed.

flake8

rbtools/api/decode.py (Diff revision 1)
The issue has been dropped. Show all issues
```
'typing.Callable' imported but unused

Column: 1
Error code: F401
```

rbtools/api/request.py (Diff revision 1)

The issue has been dropped. Show all issues

'http.client.UNAUTHORIZED' imported but unused

Column: 1
Error code: F401

rbtools/api/request.py (Diff revision 1)
The issue has been dropped. Show all issues
```
block comment should start with '# '

Column: 9
Error code: E265
```
rbtools/api/request.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (82 > 79 characters)

Column: 80
Error code: E501
```
rbtools/api/transport/sync.py (Diff revision 1)
The issue has been dropped. Show all issues
```
'typing.Union' imported but unused

Column: 1
Error code: F401
```

Commits:

	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.	3eae8188ab528607a7519411649a99ed3ed4a909
	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.	8f46dc053e8e3cbb70ae9ec1bd2f509a3b78802c

Diff:

Revision 2 (+2606 -692)

Show changes

	rbtools/api/cache.py
	rbtools/api/client.py
	rbtools/api/decode.py
	rbtools/api/decorators.py
	rbtools/api/errors.py
	rbtools/api/factory.py
	rbtools/api/request.py
	rbtools/api/utils.py
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

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

rbtools/api/cache.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This warning shouldn't be here.
```
rbtools/api/cache.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This should say "Use CachedHTTPResponse.headers instead.".
```
rbtools/api/cache.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This should say "Use CachedHTTPResponse.code instead.".
```
rbtools/api/cache.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This warning shouldn't be here.
```
rbtools/api/factory.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Could add Args: and Returns: sections.
```
rbtools/api/transport/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Forgot to add typing here.
```

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

The issue has been dropped. 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?

david Oct. 4, 2022, 8:44 a.m.

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.

Commits:

	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.	8f46dc053e8e3cbb70ae9ec1bd2f509a3b78802c
	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.	350047dedb2e13794d7ccd336db8a33d6948f6a9

Diff:

Revision 3 (+2680 -710)

Show changes

	rbtools/api/cache.py
	rbtools/api/client.py
	rbtools/api/decode.py
	rbtools/api/decorators.py
	rbtools/api/errors.py
	rbtools/api/factory.py
	rbtools/api/request.py
	rbtools/api/utils.py
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

rbtools/api/cache.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we :py:class: this?
```
rbtools/api/cache.py (Diff revision 3)
The issue has been resolved. Show all issues
```
We can use :py:attr: here.
```
rbtools/api/cache.py (Diff revision 3)
The issue has been resolved. Show all issues
```
We can use :py:attr: here.
Same with others.
```

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

The issue has been resolved. Show all issues

Properties should go before non-constructor methods.

Same below.

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

The issue has been resolved. 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.

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

The issue has been resolved. 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:

david Oct. 5, 2022, 9:16 p.m.

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.

chipx86 Oct. 6, 2022, 2:55 p.m.

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.

david Oct. 6, 2022, 4:50 p.m.
```
OK, I'll change them.
```

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

The issue has been dropped. Show all issues

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

rbtools/api/client.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Same as get_path.
```

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

The issue has been dropped. Show all issues

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

david Oct. 5, 2022, 9:16 p.m.

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.

rbtools/api/client.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Same as login.
```

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

The issue has been resolved. 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.

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

The issue has been resolved. Show all issues

This can probably just be:

decoder = DECODER_MAP.get(format, DefaultDecoder)

rbtools/api/errors.py (Diff revision 3)
The issue has been resolved. Show all issues
```
We should have one param per line here.
```

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

The issue has been resolved. Show all issues

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

rbtools/api/request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
We re-define this Union a couple times. Let's make a TypeAlias.
```
rbtools/api/request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
One parameter per line here.
```
rbtools/api/request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
One parameter per line with types.
```
rbtools/api/request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
The description can't be indented here.
```
rbtools/api/request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
RBTools 5.
```
rbtools/api/request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing a trailing comma.
```

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

The issue has been resolved. Show all issues

Can we just super() here?

david Oct. 5, 2022, 9:16 p.m.

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

rbtools/api/request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
I think we can safely use super() here.
```
rbtools/api/request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing a trailing comma.
```
rbtools/api/request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
These start at 0.
```
rbtools/api/request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we document this while here?
```

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

The issue has been resolved. 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?

rbtools/api/request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing trailing comman and return type (-> None).
```

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.

rbtools/api/transport/__init__.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing trailing comma.
Same on functions below.
```

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

The issue has been dropped. Show all issues

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

rbtools/api/transport/sync.py (Diff revision 3)
The issue has been resolved. Show all issues
```
These can easily fit one per line starting on the opening line.
```
rbtools/api/transport/sync.py (Diff revision 3)
The issue has been resolved. Show all issues
```
__name__
```
rbtools/api/transport/sync.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing trailing comma.
```

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

The issue has been resolved. Show all issues

We should document these, like our other TypedDicts.

rbtools/api/utils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
One parameter per line when typed.
```

Commits:

	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.	350047dedb2e13794d7ccd336db8a33d6948f6a9
	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.	c56aff3436dd7aad7a303a2cafb07fbb0b77d0ea

Diff:

Revision 4 (+2884 -718)

Show changes

	rbtools/api/cache.py
	rbtools/api/client.py
	rbtools/api/decode.py
	rbtools/api/decorators.py
	rbtools/api/errors.py
	rbtools/api/factory.py
	rbtools/api/request.py
	rbtools/api/utils.py
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/api/client.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Missing a trailing comma.
```
rbtools/api/client.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Missing a trailing comma.
```
rbtools/api/client.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Missing a trailing comma.
```
rbtools/api/errors.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Missing a trailing comma.
```
rbtools/api/errors.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Missing a trailing comma.
```
rbtools/api/utils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can you add a Version Added?
```

Commits:

	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.	c56aff3436dd7aad7a303a2cafb07fbb0b77d0ea
	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

Diff:

Revision 5 (+2938 -722)

Show changes

	rbtools/api/cache.py
	rbtools/api/client.py
	rbtools/api/decode.py
	rbtools/api/decorators.py
	rbtools/api/errors.py
	rbtools/api/factory.py
	rbtools/api/request.py
	rbtools/api/utils.py
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.x (ca31831)