Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add typing to filepath #4980

Merged
merged 21 commits into from Sep 16, 2021
Merged

Conversation

DanielNoord
Copy link
Member

@DanielNoord DanielNoord commented Sep 7, 2021

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

pylint/lint/expand_modules.py Outdated Show resolved Hide resolved
name, filepath, modname = file_item

if not _worker_linter:
raise Exception("Worker linter not yet initialised")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

def test_worker_check_single_file_uninitialised(self) -> None:
pylint.lint.parallel._worker_linter = None
with pytest.raises( # Objects that do not match the linter interface will fail
AttributeError, match="'NoneType' object has no attribute 'open'"
):
worker_check_single_file(_gen_file_data())

I think making this error more explicit as mypy wants makes sense. Although this is probably the wrong Exception type.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.12.0 milestone Sep 7, 2021
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a review, just some thoughts.

pylint/lint/expand_modules.py Outdated Show resolved Hide resolved
pylint/lint/parallel.py Outdated Show resolved Hide resolved
pylint/lint/parallel.py Outdated Show resolved Hide resolved
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

pylint/lint/parallel.py Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.12.0, 2.11.0 Sep 14, 2021
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Sep 14, 2021
@DanielNoord
Copy link
Member Author

@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!

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pylint/lint/expand_modules.py Outdated Show resolved Hide resolved
pylint/lint/parallel.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like where this is going!

pylint/lint/parallel.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
"""
name, filepath, modname = file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for keeping the refactors for later :)

@Pierre-Sassoulas
Copy link
Member

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

We should handle Union[Path, str] imo.

DanielNoord and others added 2 commits September 15, 2021 08:30
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
@DanielNoord
Copy link
Member Author

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
Copy link
Member

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
Copy link

coveralls commented Sep 15, 2021

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 Coverage Status
Change from base Build 1238835788: -0.006%
Covered Lines: 13313
Relevant Lines: 14302

💛 - Coveralls

@DanielNoord
Copy link
Member Author

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.

pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
pylint/typing.py Outdated Show resolved Hide resolved
DanielNoord and others added 2 commits September 16, 2021 12:42
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
@Pierre-Sassoulas Pierre-Sassoulas merged commit 0d246e5 into PyCQA:main Sep 16, 2021
32 checks passed
@DanielNoord DanielNoord deleted the typing-filepath branch September 16, 2021 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants