Page MenuHomeSoftware Heritage
Differential D2622

Add type annotations to indexer classes
ClosedPublic
Actions

Authored by krithikvaidya on Feb 4 2020, 3:31 PM.

Details

Reviewers
vlorentz
ardumont
Group Reviewers
Reviewers
Maniphest Tasks
T2258: Add type annotations to indexer classes
Commits
rDCIDX5f49b59e6aa3: Add type annotations to indexer classes
Required Signatures
L3 Software Heritage Contributor License Agreement, version 1.0
Summary

For annotating classes and methods in swh/indexer/indexer.py, swh/indexer/metadata.py, swh/indexer/mimetype.py, swh/indexer/origin_head.py and swh/indexer/rehash.py.

Diff Detail

Repository
rDCIDX Metadata indexer
Branch
typing-indexer
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10432
Build 15531: tox-on-jenkinsJenkins
Build 15530: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
krithikvaidya added a comment.EditedFeb 4 2020, 4:20 PM

Okay great! I'll add the type annotations to the other files too. Thanks

swh-public-ci added a comment.Feb 4 2020, 4:24 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/677/ for more details.

Harbormaster completed remote builds in B10437: Diff 9376.Feb 4 2020, 4:24 PM
krithikvaidya retitled this revision from Add type annotations for mimetype.py to Add type annotations to indexer classes.EditedFeb 5 2020, 7:14 PM
krithikvaidya edited the summary of this revision. (Show Details)

In swh-indexer's indexer.py (code), the run() method of BaseIndexer class has been overriden by a few derived classes. However, I think some of these overridings may be flawed, since when I try to type-annotate them, they're giving the mypy errors: argument of method incompatible with supertype / Signature of "run" incompatible with supertype "BaseIndexer". This is probably because of a violation of the Liskov substitution principle.

Do you'll suggest that I just tell mypy to ignore these errors for now? I can't get mypy to ignore these errors in the method definition expression either. The expression is spread across multiple lines, and the ignore annotation works for these kinds of expressions only in Python 3.8+ (source). So, I could just remove type checking from these methods altogether, for now.

These are the errors I'm getting:

krithikvaidya added a comment.Feb 6 2020, 12:39 PM

Not sure if adding a comment notifies the reviewers, so tagging @vlorentz @ardumont

vlorentz added a comment.Feb 6 2020, 1:12 PM

They are errors, I don't think we should ignore them, but fix the underlying issue;

vlorentz requested changes to this revision.Feb 6 2020, 1:13 PM

(I'm marking this diff as "changes requested" to reflect that you're planning on making changes)

This revision now requires changes to proceed.Feb 6 2020, 1:13 PM
krithikvaidya planned changes to this revision.Feb 6 2020, 1:19 PM
In D2622#62664, @vlorentz wrote:

They are errors, I don't think we should ignore them, but fix the underlying issue;

Right... So I'll remove the type annotations from these methods for now. Once I'm done, should I create a separate Maniphest task describing the underlying issue?

krithikvaidya updated this revision to Diff 9420.EditedFeb 6 2020, 5:25 PM

Add type annotations for rehash.py, indexer.py, origin_head.py and metadata.py.

Overriden methods and their parent class methods have mostly been excluded from type annotating, for now.
A lot of the methods in origin_head.py require documentation and are incomplete/unused, so I've had to use a lot of Any's in typing those.

I've set the type annotation as Any if

  • I'm unsure of the type
  • The field has values of different types, like in the value field of some Dict's or in some elements of some Tuple's
  • The type of the variable changes in the program.
swh-public-ci added a comment.Feb 6 2020, 5:32 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/678/ for more details.

Harbormaster completed remote builds in B10479: Diff 9420.Feb 6 2020, 5:32 PM
krithikvaidya updated this revision to Diff 9421.Feb 6 2020, 5:49 PM

Apply minor, cosmetic changes

swh-public-ci added a comment.Feb 6 2020, 5:55 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/679/ for more details.

Harbormaster completed remote builds in B10480: Diff 9421.Feb 6 2020, 5:55 PM
krithikvaidya added a comment.Feb 6 2020, 6:00 PM

@vlorentz @ardumont
I think I'm done here, please check the code and my comments above.

swh/indexer/indexer.py
472 ↗(On Diff #9421)

Not sure which overriden index() is being called here

ardumont accepted this revision.Feb 7 2020, 3:13 AM

Thanks, looks good.

Remains some remarks.
And another one more general, for the docstring, either wrap all parameters with **parameter-name**: description or remove them altogether.
I see you kinda mix those.

swh/indexer/indexer.py
39 ↗(On Diff #9421)

Add the working directory docstring, it's where the file is written.
i also think it's a string (to update its type in the signature).

472 ↗(On Diff #9421)

yeah, it's fine for now [1]

The one that matters here is the main class BaseIndex's index definition which you typed.
It's highly possible the subclasses are not consistent on the type, only on the parameters so, fine.
everything is fine ;)

[1] indexer has grown a bit and after a while and getting back to it, it's a tad unclear.

486 ↗(On Diff #9421)

remove the **

swh/indexer/mimetype.py
137

str without the quote :)

krithikvaidya added a comment.Feb 7 2020, 4:13 AM
In D2622#62794, @ardumont wrote:

And another one more general, for the docstring, either wrap all parameters with **parameter-name**: description or remove them altogether.

Done. I've left the single ** for the kwargs in the docstring though.

In D2622#62671, @krithikvaidya wrote:

Once I'm done, should I create a separate Maniphest task describing the underlying issue?

Should I do this?

swh/indexer/indexer.py
39 ↗(On Diff #9421)

Added to the docstring.

Regarding the type of working_directory, I wasn't able to figure it out based on checking its use in this method, as well as from checking the methods that call this method through using grep. According to this, the type can either be either a string or bytes object representing a path. . So if we're sure it's a string, I'll change it to str 👍

472 ↗(On Diff #9421)

Ah I see

krithikvaidya marked 6 inline comments as done.Feb 7 2020, 4:14 AM
vlorentz added a comment.Feb 7 2020, 11:05 AM

You should use a TypeVar for the type of results, so you don't have to use Any every time.

swh/indexer/indexer.py
33 ↗(On Diff #9421)

it yields strings

186 ↗(On Diff #9421)

annotation should preferably be on the class itself.

189 ↗(On Diff #9421)

It's a Dict

257 ↗(On Diff #9421)

it yields bytes

453 ↗(On Diff #9421)

it yields dicts

482 ↗(On Diff #9421)

yields bytes

vlorentz requested changes to this revision.Feb 7 2020, 11:08 AM
vlorentz added inline comments.
swh/indexer/origin_head.py
67 ↗(On Diff #9421)

can be improved: https://docs.python.org/3/library/typing.html#typing.Tuple

111 ↗(On Diff #9421)

not a bool

123 ↗(On Diff #9421)

You can use specific types

swh/indexer/rehash.py
86 ↗(On Diff #9421)

Generator

This revision now requires changes to proceed.Feb 7 2020, 11:08 AM
krithikvaidya marked 9 inline comments as done.Feb 7 2020, 1:46 PM
In D2622#62814, @vlorentz wrote:

You should use a TypeVar for the type of results, so you don't have to use Any every time.

Sorry didn't get you. What results are you talking about?

swh/indexer/indexer.py
482 ↗(On Diff #9421)

I'm not sure that this is the case, since this method yields from _index_contents(), and _index_contents() yields Dicts.

vlorentz added a comment.Feb 7 2020, 3:16 PM
In D2622#62892, @krithikvaidya wrote:

Sorry didn't get you. What results are you talking about?

All variables named results

swh/indexer/indexer.py
482 ↗(On Diff #9421)

Then you should also fix the docstring

krithikvaidya updated this revision to Diff 9442.Feb 7 2020, 4:57 PM

Requested changes made

krithikvaidya marked 3 inline comments as done.Feb 7 2020, 4:59 PM
vlorentz requested changes to this revision.Feb 7 2020, 5:01 PM
vlorentz added inline comments.
swh/indexer/indexer.py
453 ↗(On Diff #9421)

SendType should be None, not Any. https://docs.python.org/3/library/typing.html#typing.Generator

482 ↗(On Diff #9421)

same comment on SentType

swh/indexer/metadata.py
29 ↗(On Diff #9442)

sorry I missed this earlier, but you also forgot yield type for this generator

This revision now requires changes to proceed.Feb 7 2020, 5:01 PM
swh-public-ci added a comment.Feb 7 2020, 5:04 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/680/ for more details.

Harbormaster completed remote builds in B10502: Diff 9442.Feb 7 2020, 5:04 PM
krithikvaidya updated this revision to Diff 9448.Feb 7 2020, 6:26 PM

Fix minor issues

krithikvaidya marked 3 inline comments as done.Feb 7 2020, 6:31 PM
swh-public-ci added a comment.Feb 7 2020, 6:33 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/681/ for more details.

Harbormaster completed remote builds in B10508: Diff 9448.Feb 7 2020, 6:33 PM
ardumont added inline comments.Feb 7 2020, 6:50 PM
swh/indexer/indexer.py
39 ↗(On Diff #9421)

Yes, it could be bytes also.
but i believe we only use it with string.

krithikvaidya updated this revision to Diff 9449.Feb 7 2020, 6:55 PM

Change type from Any to str

krithikvaidya marked an inline comment as done.Feb 7 2020, 6:56 PM
swh-public-ci added a comment.Feb 7 2020, 7:02 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/682/ for more details.

Harbormaster completed remote builds in B10509: Diff 9449.Feb 7 2020, 7:02 PM
vlorentz requested changes to this revision.Feb 7 2020, 11:17 PM

Looks good! Now you just need to squash your commits before I accept the diff

This revision now requires changes to proceed.Feb 7 2020, 11:17 PM
krithikvaidya updated this revision to Diff 9450.Feb 8 2020, 4:20 AM

Squashed commits

swh-public-ci added a comment.Feb 8 2020, 4:26 AM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/683/ for more details.

Harbormaster completed remote builds in B10510: Diff 9450.Feb 8 2020, 4:26 AM
krithikvaidya updated this revision to Diff 9451.Feb 8 2020, 6:28 AM

Update commit message

swh-public-ci added a comment.Feb 8 2020, 6:34 AM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/684/ for more details.

Harbormaster completed remote builds in B10511: Diff 9451.Feb 8 2020, 6:34 AM
vlorentz accepted this revision.Feb 10 2020, 12:11 PM
This revision is now accepted and ready to land.Feb 10 2020, 12:11 PM
vlorentz added a comment.Feb 10 2020, 12:11 PM

Thanks!

krithikvaidya updated this revision to Diff 9468.Feb 11 2020, 4:27 AM

Rebase branch on master

swh-public-ci added a comment.Feb 11 2020, 4:34 AM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/685/ for more details.

Harbormaster completed remote builds in B10531: Diff 9468.Feb 11 2020, 4:34 AM
Closed by commit rDCIDX5f49b59e6aa3: Add type annotations to indexer classes (authored by krithikvaidya). · Explain WhyFeb 11 2020, 4:35 AM
This revision was automatically updated to reflect the committed changes.
krithikvaidya added a commit: rDCIDX5f49b59e6aa3: Add type annotations to indexer classes.
krithikvaidya added a comment.Feb 11 2020, 4:37 AM

Great! Thanks for the patience and review 😃 . Will work on another task after my exams are done 😴

In D2622#62671, @krithikvaidya wrote:
In D2622#62664, @vlorentz wrote:

They are errors, I don't think we should ignore them, but fix the underlying issue;

Right... So I'll remove the type annotations from these methods for now. Once I'm done, should I create a separate Maniphest task describing the underlying issue?

This issue still remains, though

vlorentz added a comment.Feb 11 2020, 10:25 AM

yes, thanks

Revision Contents
Changeset List

PathSize
5 lines
swh/
indexer/
24 lines

Diff 9371

View Options

.pre-commit-config.yaml

Loading...
View Options

swh/indexer/mimetype.py

Loading...
About · Developer information · Legal