Hi, I'd like to take up this issue as my first issue here :) . But before I take it up, I just had a few queries:

• The pytest tests are succeeding in the swh-indexer module, but failing in some other modules. Since this issue pertains to only the swh-indexer module, it shouldn't cause problems, right?
• Are we looking to add just function annotations, or variable annotations (Python 3.6+) too?
• I'm going through the code review workflow guide and the Git style guide. Is there anything else I need to know?

The pytest tests are succeeding in the swh-indexer module, but failing in some other modules. Since this issue pertains to only the swh-indexer module, it shouldn't cause problems, right?

They should not be failing and they don't [1] [2]
It should not cause problem either for this task.

We use mainly tox (from top-level repository directory) to run the pytest tests (this pulls dependencies and installs them in a sandbox so the runtime system is not polluting the tests runtime).
We sometimes use directly pytest as well (when we want to check impacts from other modules' current development change).
So i'd suggest you use tox as well.

Are we looking to add just function annotations, or variable annotations (Python 3.6+) too?

Functions and methods, yes.
Sometimes, to satisfy mypy, we need to also type variables.
Reasonably, as our ci [1] runs those and we want to keep it green.
So in the end, you want the tox -e mypy to remain green.
So any warning this command raises should be fixed ;)

I'm going through the code review workflow guide and the Git style guide. Is there anything else I need to know?

Nothing else comes to mind for now.
But, do not worry, that will be discussed during the code review step ;)

[1] https://jenkins.softwareheritage.org/

[2] storage is currently gray because, most possibly, as we merged a new cassandra backend implementation, tests are taking way more time than before. That's a temporary/transitional issue independent from the swh-indexer indeed.

Cheers,

Thanks for the detailed reply 🙂, and apologies for the delay. Things are clearer now 👍

We use mainly tox (from top-level repository directory) to run the pytest tests (this pulls dependencies and installs them in a sandbox so the runtime system is not polluting the tests runtime).

So, say I make changes to one module (say swh-indexer), and the tox tests for this module pass locally. Then, when this code passes through the CI pipeline, the tests there will definitely pass, right? (irrespective of whether the direct pytest tests pass locally or not)

So in the end, you want the tox -e mypy to remain green.

Got it. So as I update the methods and functions with type annotations, I'll keep running these changes through mypy. In case just annotating the function/method header isn't sufficient to pass the mypy tests, I'll type the variables too.

Anyways, I'm nearly done with adding the annotations to one of the files (swh/indexer/mimetype.py). Once I'm done with this file, can I submit a diff for just these changes? Just so that I know I'm doing it right before moving on to the other files.

and apologies for the delay.

no worries there.

Thanks for the detailed reply 🙂,
Things are clearer now 👍

awesome

So, say I make changes to one module (say swh-indexer), and the tox tests for this module pass locally. Then, when this code passes through the CI pipeline, the tests there will definitely pass, right? (irrespective of whether the direct pytest tests pass locally or not)

yes

Got it. So as I update the methods and functions with type annotations, I'll keep running these changes through mypy. In case just annotating the function/method header isn't sufficient to pass the mypy tests, I'll type the variables too.

yes

Anyways, I'm nearly done with adding the annotations to one of the files (swh/indexer/mimetype.py). Once I'm done with this file, can I submit a diff for just these changes? Just so that I know I'm doing it right before moving on to the other files.

yes, you can open the diff while in-progress.
So you will have a better understanding of what's happening after opening the diff.
Check if jenkins is working as you think it should (and checking the logs if not).
And then you can always mark the diff as "planning changes" so we do not start the review too early ;)

Cheers,

Great, understood

And does Jenkins also run code in non-master branches through the pipeline?

Yes, but you should just open a diff instead.