Add typing to filepath

[DanielNoord]

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Write a good description on what the PR does.

Type of Changes

	Type
✓	🔨 Refactoring

Description

This fails the test suite, but I think this might be because the test suite is testing something that doesn't happen in normal use of pylint. Please correct me if I am wrong but after investigating the call-chain I believe that almost all filepath's handled by pylint are str (the only exception being get_fatal_error_message()). This presumes that no user directly calls linter.check() as we do in the test suite.
Of note is lint.py_run(), which I believe also does no run check() before calling load_command_line_configuration.

The following reasoning leads me to believe that a normal use of pylint always creates a str file path.

• load_command_line_configuration() in pylint/config/option_manager_mixin.py always returns a List[str]. Therefore the args variable in the __init__ of the Run class in pylint/lint/run.py is always a List[str].
• Subsequently in pylint/lint/pylinter.py the files_or_modules argument to check() is always a List[str]. Which means that both check_parallel() and _check_files() only handle str representations of paths.
• Therefore set_current_module() and on_set_current_module() in various other classes only get given str.

DanielNoord#10
This PR shows that with changes to the tests this PR can succeed. These changes are:

• Making sure all initial file paths are a List[str]
• Making sure that test_worker_check_single_file_uninitialised() catches the new explicitly defined exception
• Making sure that get_fatal_error_message() always gets a Path as second argument
• Making sure that check_parallel() always gets an Iterable

These changes seem to follow the standard format of these arguments/variables in normal runs of pylint that are being skipped in the tests.

[DanielNoord]

mpy complains on the following two lines about the possibility that _work_linter might be None.
We currently already test for this, but do so with catching an AttributeError on L73. See:
https://github.com/PyCQA/pylint/blob/fab2de272b3fc36dc4480d89840b8bfe0fbfbd0c/tests/test_check_parallel.py#L177-L182
I think making this error more explicit as mypy wants makes sense. Although this is probably the wrong Exception type.

[cdce8p]

Not a review, just some thoughts.

[cdce8p]

I wouldn't necessarily wait for #4973 to add typing.py. You could also add it here. It shouldn't be too hard to resolve the merge conflicts in the end.

For Tuple[str, str, str] it might even make sense to create a separate one. You should you by now that I like small PRs 😄

[DanielNoord]

@cdce8p I added the NamedTuple. However, doing this separately was (quite) awful because of the various new mypy errors that I already fixed in this PR. (For example raising the Exception on if not _worker_linter).
So I added it here!

[cdce8p]

I haven't commented on that before. A best practice when typing in python is to use the most generic type possible for arguments and the most concrete one for return types. I.e.

def func(var: Iterable[str]) -> List[str]:
    ...

If it doesn't matter if a list, tuple or something else is pass in as long as it's possible to iterate over it, it shouldn't matter to the type checker either.

[cdce8p]

I really like where this is going!

[cdce8p]

Why do a tuple unpacking. With NamedTuple you can access the attributes by name.
It might even make sense to just pass file to set_current_module / get_ast and do the attribute access there. A bit more refactoring though.

[DanielNoord]

I left the refactor for another time, since this is becoming quite a large PR.

One thing I noticed is some ambiguity between modname and modpath. They seem to be used for the same attribute. We currently use modpath, is that okay? Or do you want to start using modname?

[Pierre-Sassoulas]

+1 for keeping the refactors for later :)

[Pierre-Sassoulas]

I believe that almost all filepath's handled by pylint are str

We should handle Union[Path, str] imo.

[DanielNoord]

We should handle Union[Path, str] imo.

Not sure if I agree here. This would probably lead us to having to add Path to many other typings down the line. The current flow of pylint clearly does not allow Path to be passed. With a little refactor we can even get the last instance of Path to also become a str. Having to add Union[Path, str] instead of str everywhere might clutter the codebase.

@cdce8p & @Pierre-Sassoulas Do you guys thinks your review is far enough for me to add the commit with test fixes as discussed in the PR description and shown in DanielNoord#10?

[Pierre-Sassoulas]

might clutter the codebase.

Yes, also the migration to Path is also a huge endeavor with not a lot of clear benefit right now. Most os.path handles Union[Path, str] so technically that's what we can probably handle on most of the function, but finding out where we can't is probably not worth the time and crashes it will create.

[coveralls]

Pull Request Test Coverage Report for Build 1241243212

• 70 of 73 (95.89%) changed or added relevant lines in 10 files are covered.
• 18 unchanged lines in 5 files lost coverage.
• Overall coverage decreased (-0.006%) to 93.085%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/lint/pylinter.py	26	29	89.66%

Files with Coverage Reduction	New Missed Lines	%
pylint/interfaces.py	1	97.06%
pylint/reporters/reports_handler_mix_in.py	2	95.45%
pylint/reporters/ureports/nodes.py	3	94.29%
pylint/checkers/imports.py	6	94.59%
pylint/reporters/ureports/text_writer.py	6	90.16%

Totals
Change from base Build 1238835788:	-0.006%
Covered Lines:	13313
Relevant Lines:	14302

---

💛 - Coveralls

[DanielNoord]

This now includes the relevant changes to the tests to make this work. As discussed in PR description I believe the tests check things that cannot happen in real world scenarios. The description includes some additional reasoning for this and a description of the changes to the tests.