Page MenuHomeSoftware Heritage

add annotation type in some lister file
ClosedPublic

Authored by yanng23 on Feb 17 2020, 4:03 PM.

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

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

This revision now requires changes to proceed.Feb 18 2020, 10:53 AM
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

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]

This revision now requires changes to proceed.Feb 20 2020, 1:32 PM
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 {}

ardumont added a subscriber: ardumont.

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.
So here it's Optional[str].

Same goes for override_config, Optional[bool].

72–74

why the type: ignore?

it's possible some implementation are inconsistent.
In that case, you need to improve on the identifier's type.

Union[datetime, <other-type>, etc...].

swh/lister/cgit/lister.py
58

You need to type all listers...

This revision now requires changes to proceed.Mar 24 2020, 6:32 PM
swh/lister/cgit/lister.py
99

you can use Iterator[str]

ardumont retitled this revision from add anotation type in some lister file to add annotation type in some lister file.Mar 27 2020, 11:02 AM
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...
but then it's shadowed to something else (with maybe another type) used by sqlalchemy...
so mypy won't be happy unless we change the key names to something else in the body method.

The value could be anything.
And the return result is whatever sqlalchemy returns, a row of some sort...

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

vlorentz added inline comments.
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.

This revision is now accepted and ready to land.Mar 31 2020, 4:33 PM

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

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

Oh yes, my firstname is Yann and my name Gautier, thanks :)

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.