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
Add typing to filepath
#4980
Changes from 7 commits
dc3696f
56b1061
565e245
2b46031
a680a4a
7bd3810
f4a7045
6af1902
ccaaa86
4c31342
4859288
b8ba102
096f8a8
0910978
dd67951
32742d2
cfc01da
21e6f76
4df05c5
58d0e88
0cf4b1f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
import traceback | ||
import warnings | ||
from io import TextIOWrapper | ||
from typing import Iterable, Iterator, List, Optional, Sequence | ||
|
||
import astroid | ||
from astroid import AstroidError, nodes | ||
|
@@ -31,6 +32,7 @@ | |
) | ||
from pylint.message import MessageDefinitionStore, MessagesHandlerMixIn | ||
from pylint.reporters.ureports import nodes as report_nodes | ||
from pylint.typing import FileItem, ModuleDescriptionDict | ||
from pylint.utils import ASTWalker, FileState, utils | ||
from pylint.utils.pragma_parser import ( | ||
OPTION_PO, | ||
|
@@ -935,16 +937,13 @@ def initialize(self): | |
if not msg.may_be_emitted(): | ||
self._msgs_state[msg.msgid] = False | ||
|
||
def check(self, files_or_modules): | ||
def check(self, files_or_modules: Sequence[str]) -> None: | ||
DanielNoord marked this conversation as resolved.
Show resolved
Hide resolved
cdce8p marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"""main checking entry: check a list of files or modules from their name. | ||
|
||
files_or_modules is either a string or list of strings presenting modules to check. | ||
files_or_modules is a list of strings presenting modules to check. | ||
DanielNoord marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" | ||
self.initialize() | ||
|
||
DanielNoord marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if not isinstance(files_or_modules, (list, tuple)): | ||
files_or_modules = (files_or_modules,) | ||
|
||
Pierre-Sassoulas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if self.config.from_stdin: | ||
if len(files_or_modules) != 1: | ||
raise exceptions.InvalidArgsError( | ||
|
@@ -970,19 +969,21 @@ def check(self, files_or_modules): | |
files_or_modules, | ||
) | ||
|
||
def check_single_file(self, name, filepath, modname): | ||
def check_single_file(self, file: FileItem) -> None: | ||
cdce8p marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"""Check single file | ||
|
||
The arguments are the same that are documented in _check_files | ||
|
||
The initialize() method should be called before calling this method | ||
""" | ||
with self._astroid_module_checker() as check_astroid_module: | ||
self._check_file( | ||
self.get_ast, check_astroid_module, name, filepath, modname | ||
) | ||
self._check_file(self.get_ast, check_astroid_module, file) | ||
|
||
def _check_files(self, get_ast, file_descrs): | ||
def _check_files( | ||
self, | ||
get_ast, | ||
file_descrs: Iterable[FileItem], | ||
) -> None: | ||
"""Check all files from file_descrs | ||
|
||
The file_descrs should be iterable of tuple (name, filepath, modname) | ||
cdce8p marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
@@ -995,7 +996,7 @@ def _check_files(self, get_ast, file_descrs): | |
for name, filepath, modname in file_descrs: | ||
cdce8p marked this conversation as resolved.
Show resolved
Hide resolved
|
||
try: | ||
self._check_file( | ||
get_ast, check_astroid_module, name, filepath, modname | ||
get_ast, check_astroid_module, FileItem(name, filepath, modname) | ||
) | ||
except Exception as ex: # pylint: disable=broad-except | ||
template_path = prepare_crash_report( | ||
|
@@ -1004,23 +1005,22 @@ def _check_files(self, get_ast, file_descrs): | |
msg = get_fatal_error_message(filepath, template_path) | ||
if isinstance(ex, AstroidError): | ||
symbol = "astroid-error" | ||
msg = (filepath, msg) | ||
self.add_message(symbol, args=(filepath, msg)) | ||
else: | ||
symbol = "fatal" | ||
self.add_message(symbol, args=msg) | ||
self.add_message(symbol, args=msg) | ||
|
||
def _check_file(self, get_ast, check_astroid_module, name, filepath, modname): | ||
def _check_file(self, get_ast, check_astroid_module, file: FileItem): | ||
cdce8p marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"""Check a file using the passed utility functions (get_ast and check_astroid_module) | ||
|
||
:param callable get_ast: callable returning AST from defined file taking the following arguments | ||
- filepath: path to the file to check | ||
- name: Python module name | ||
:param callable check_astroid_module: callable checking an AST taking the following arguments | ||
- ast: AST of the module | ||
:param str name: full name of the module | ||
:param str filepath: path to checked file | ||
:param str modname: name of the checked Python module | ||
:param FileItem file: data about the file | ||
""" | ||
name, filepath, modname = file | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do a tuple unpacking. With There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for keeping the refactors for later :) |
||
self.set_current_module(name, filepath) | ||
# get the module representation | ||
ast_node = get_ast(filepath, name) | ||
|
@@ -1042,7 +1042,7 @@ def _check_file(self, get_ast, check_astroid_module, name, filepath, modname): | |
self.add_message(msgid, line, None, args) | ||
|
||
@staticmethod | ||
def _get_file_descr_from_stdin(filepath): | ||
def _get_file_descr_from_stdin(filepath: str) -> FileItem: | ||
"""Return file description (tuple of module name, file path, base name) from given file path | ||
|
||
This method is used for creating suitable file description for _check_files when the | ||
|
@@ -1056,19 +1056,19 @@ def _get_file_descr_from_stdin(filepath): | |
except ImportError: | ||
modname = os.path.splitext(os.path.basename(filepath))[0] | ||
|
||
return (modname, filepath, filepath) | ||
return FileItem(modname, filepath, filepath) | ||
Pierre-Sassoulas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def _iterate_file_descrs(self, files_or_modules): | ||
def _iterate_file_descrs(self, files_or_modules) -> Iterator[FileItem]: | ||
"""Return generator yielding file descriptions (tuples of module name, file path, base name) | ||
|
||
The returned generator yield one item for each Python module that should be linted. | ||
""" | ||
for descr in self._expand_files(files_or_modules): | ||
name, filepath, is_arg = descr["name"], descr["path"], descr["isarg"] | ||
if self.should_analyze_file(name, filepath, is_argument=is_arg): | ||
yield (name, filepath, descr["basename"]) | ||
yield FileItem(name, filepath, descr["basename"]) | ||
|
||
def _expand_files(self, modules): | ||
def _expand_files(self, modules) -> List[ModuleDescriptionDict]: | ||
"""get modules and errors from a list of modules and handle errors""" | ||
result, errors = expand_modules( | ||
modules, | ||
|
@@ -1085,7 +1085,7 @@ def _expand_files(self, modules): | |
self.add_message(key, args=message) | ||
return result | ||
|
||
def set_current_module(self, modname, filepath=None): | ||
def set_current_module(self, modname, filepath: Optional[str] = None): | ||
"""set the name of the currently analyzed module and | ||
init statistics for it | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html | ||
# For details: https://github.com/PyCQA/pylint/blob/main/LICENSE | ||
|
||
"""A collection of typing utilities.""" | ||
import sys | ||
from typing import NamedTuple, Union | ||
|
||
if sys.version_info >= (3, 8): | ||
from typing import Literal, TypedDict | ||
else: | ||
from typing_extensions import Literal, TypedDict | ||
|
||
|
||
class FileItem(NamedTuple): | ||
"""Represents data about a file handled by pylint""" | ||
|
||
name: str | ||
filepath: str | ||
modpath: str | ||
|
||
|
||
class ModuleDescriptionDict(TypedDict): | ||
"""Represents data about a checked module""" | ||
|
||
path: str | ||
name: str | ||
isarg: bool | ||
basepath: str | ||
basename: str | ||
|
||
|
||
class ErrorDescriptionDict(TypedDict): | ||
"""Represents data about errors collected during checking of a module""" | ||
|
||
key: Literal["fatal"] | ||
mod: str | ||
ex: Union[ImportError, SyntaxError] |
There was a problem hiding this comment.
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 beNone
.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 wrongException
type.