Details
- Reviewers
vlorentz ardumont - Group Reviewers
Reviewers - Commits
- rDLSe5fea84c5565: review corrections
rDLS60adc424be11: add anotation type in some lister file - Required Signatures
L3 Software Heritage Contributor License Agreement, version 1.0
Diff Detail
- Repository
- rDLS Listers
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/538/ for more details.
Hi,
Thanks for the Diff. I started reviewing the Diff, and I have a bunch of comments, see below.
Many of these comments apply to other parts of the diff as well, please fix these as well.
swh/lister/bitbucket/lister.py | ||
---|---|---|
48 | Optional[datetime] | |
64 | not needed | |
swh/lister/cgit/lister.py | ||
10 | ? | |
58–68 | why did you remove annotations? | |
99 | please specify completely the type of the Generator | |
121 | Optional[Dict[str, Any]] | |
129 | not needed | |
swh/lister/core/indexing_lister.py | ||
61 | missing return type | |
173 | not needed | |
184 | not needed | |
187–189 | why the reformatting? | |
swh/lister/core/lister_transports.py | ||
52 | missing annotation | |
92 | missing annotation |
swh/lister/bitbucket/lister.py | ||
---|---|---|
64 | I added this return case because when I annotate the function mypy raise an error about a missing return case. So I think it's a better practice to manage all return cases, but if you prefer I can remove it and just put a "type ignore". |
swh/lister/bitbucket/lister.py | ||
---|---|---|
64 | Indeed. Forget my comment then |
Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/539/ for more details.
I see you did not annotate some of the functions. Do you need help with those?
Some more comments (on all the diff this time):
swh/lister/bitbucket/lister.py | ||
---|---|---|
82 | mypy found an error here; you should fix it instead of ignoring | |
swh/lister/core/indexing_lister.py | ||
61 | The return type is technically correct, but only because you exhaustively listed what current subclasses use. Instead, consider using generics (see https://docs.python.org/3/library/typing.html ) | |
swh/lister/core/lister_base.py | ||
196 | same | |
276 | I don't think it should be optional; self.MAX_RETRIES should always be at least 1. | |
323 | Any annotations don't provide any info | |
426–429 | why this change? | |
swh/lister/core/lister_transports.py | ||
98 | Why is type: ignore needed? | |
119–121 | I don't think you need type: ignore here; just add an annotation when defining params | |
swh/lister/npm/lister.py | ||
28 | You can be more specific than Any. | |
64 | You can probably be more specific than Any | |
137 | missing return type | |
swh/lister/phabricator/lister.py | ||
98 | missing types | |
152 | missing return type | |
swh/lister/pypi/lister.py | ||
41 | List[Dict] |
swh/lister/bitbucket/lister.py | ||
---|---|---|
82 | I don't know how to fix it. Mypy saids that "Unsupported operand types for <= ("int" and "None")", but due to the if case this error should not occur, it's why I choose to ignore it. How can I fix it ? | |
swh/lister/core/lister_base.py | ||
276 | This method returns always None in gnu/lister.py, so optional is needed. | |
426–429 | because the line is 80. | |
swh/lister/core/lister_transports.py | ||
119–121 | I don't know how fix it, mypy saids unexpected type declaration when I try to annotate the line 119. And when I annotate params when defining, mypy doesn't take care of it and the error stays incompatible type assignement. |
swh/lister/bitbucket/lister.py | ||
---|---|---|
82 | Oh indeed, I misunderstood the conditionals. You could do it like this to work around mypy: if lower is None: if upper is None: ... else: ... else: if upper is None: ... else: ... (I also find this more readable, personally) | |
swh/lister/core/lister_base.py | ||
276 | ok | |
swh/lister/core/lister_transports.py | ||
119–121 | What if you declare params like this? params: Dict[str, Any] = {} else, mypy assumes the type of params from the first value assignment, ie. params['headers'] = self.request_headers() or {} |
Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/542/ for more details.
Thanks for your work on this.
There remains some inconsistencies. Can you please fix those?
Thanks in advance.
.vscode/settings.json | ||
---|---|---|
5 ↗ | (On Diff #10242) | You need to remove this file as this is specific to your ide. |
swh/lister/bitbucket/lister.py | ||
29–30 | If it can be None, it must be Optional. Same goes for override_config, Optional[bool]. | |
72–74 | why the type: ignore? it's possible some implementation are inconsistent. Union[datetime, <other-type>, etc...]. | |
swh/lister/cgit/lister.py | ||
58 | You need to type all listers... |
Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/543/ for more details.
swh/lister/cgit/lister.py | ||
---|---|---|
99 | you can use Iterator[str] |
swh/lister/core/lister_base.py | ||
---|---|---|
323 | tbf, it's not that easy to determine the type now. key seems to be mostly a string... The value could be anything. So for me, the method is just missing a -> Any and it'd be fine as a first approximation. @vlorentz ^ what do you think? | |
404 | those are str except for c being an Optional[str] one |
swh/lister/core/lister_base.py | ||
---|---|---|
323 | ok then |
Let's land this and we'll incrementally improve from there.
Thanks for your work on this.
\o/
We forgot to mention that you can add yourself to the CONTRIBUTORS file ;)
I can do it but i don't know your firstname (i see Yann in your profile) and name.
We forgot to mention that you can add yourself to the CONTRIBUTORS file ;)
I can do it but i don't know your firstname (i see Yann in your profile) and name.Oh yes, my firstname is Yann and my name Gautier, thanks :)
Added.