feat(api)!: Make RESTObjectList typing generic #3121
Conversation
|
I made this simple python file to check type hints: import typing
from gitlab.v4.objects import MergeRequestManager
def test_list(mang: MergeRequestManager) -> None:
for m in mang.list():
typing.reveal_type(m)When run on main branch the revealed type will be: Using this branch the revealed type will be: I believe this is because both |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3121 +/- ##
=======================================
Coverage 97.28% 97.28%
=======================================
Files 97 97
Lines 5975 5976 +1
=======================================
+ Hits 5813 5814 +1
Misses 162 162
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Thanks @igorp-collabora again! This LGTM.
Could you maybe add a breaking change footer as outlined in https://www.conventionalcommits.org/en/v1.0.0/, with a short sentence outlying the return types for list endpoints will now be the real object rather than RESTObject?
This way it should be easier for downstream users to migrate to our new major bump (I'll look at other deprecations in our code and maybe add more breaking changes as well this month).
77cb2e9 to
a24c2cf
Compare
BREAKING CHANGE: Type narrowing of `list()` methods return objects from RESTObject to a concrete subclass (for example `MergeRequest`) can become redundant. Currently the RESTObjectList type hints yielded objects as base RESTObject. However, the ListMixin is now generic and will return the RESTObject subclass based on the RESTManager typing. Using `typing.Generic` it is possible to make RESTObjectList type hint a specific subclass of the RESTObject. Iterating over `list()` call the ListMixin will now yield the same object class because both `list` and `RESTObjectList` will have the same type hinted subclass. Signed-off-by: Igor Ponomarev <[email protected]>
a24c2cf to
d290a21
Compare
|
I added the breaking changes footer to the commit message and an exclamation mark to the commit summary. |
BREAKING CHANGE: Type narrowing of
list()methods return can become redundant.Currently the RESTObjectList type hints yielded objects as base RESTObject. However, the ListMixin is now generic and will return the RESTObject subclass based on the RESTManager typing.
Using
typing.Genericit is possible to make RESTObjectList type hint a specific subclass of the RESTObject.Iterating over
list()call the ListMixin will now yield the same object class because bothlistandRESTObjectListwill have the same type hinted subclass.Closes #2062