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 pylint.pyreverse.inspector #6614

Merged
merged 3 commits into from
May 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 44 additions & 23 deletions pylint/pyreverse/inspector.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,31 @@
Try to resolve definitions (namespace) dictionary, relationship...
"""

from __future__ import annotations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
from __future__ import annotations
from __future__ import annotations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some day I will learn this! 😄
Maybe that could be a pre-commit check or pylint extension as well?

Copy link
Member

Choose a reason for hiding this comment

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

I'm curious, what is the source for this convention?

Copy link
Member

Choose a reason for hiding this comment

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

PEP8 seems to say the opposite (emphasis mine): Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's my interpretation of PEP8's: You should put a blank line between each group of imports. Since the module docstring is certainly not the same "group" as the future import it makes sense to separate them just like you separate the last import from the first definition.

Note the example just below your quote:

"""This is the example module.

This module does stuff.
"""

from __future__ import barry_as_FLUFL

__all__ = ['a', 'b', 'c']
__version__ = '0.1'
__author__ = 'Cardinal Biggles'

import os
import sys

https://peps.python.org/pep-0008/ for reference.

But, I agree this is somewhat arbitrary. There does seem to be consensus though as psf/black#2996 seems to have general support and would help us with this as well.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah I wondered if that's what you were picking up on. I don't think the document is very clear. I think it's not clear that it's incompatible with "put a blank line before each subsequent group of imports". I'm not going to open a PR against PEP8 (😅 ) but maybe I'll chime in on the black issue, thanks for posting it! EDIT: nah, your example is pretty clear. Thanks for pointing me to it!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do think a suggestion to edit the PEP and linking to black wouldn't hurt. Just to see if there is actual agreement on it.

If black is going to enforce it it will be pretty "standard" anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm feeling we shouldn't enforce this style in pylint or do comments on it. If black changes, we'll get it for free. That example is showing a blank line at the top, but it's not "between" two groups of imports, it's between a docstring, so it's just a random blank line. But no big deal either way obviously.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fine with me! Agree that increasing maintenance churn for something that is likely auto-fixed by black in a couple of weeks is not worth the effort!

Copy link
Member

Choose a reason for hiding this comment

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

isort is the auto-formatter that should enforce this I think.


import collections
import os
import traceback
from collections.abc import Generator
from typing import Any, Callable, Optional

import astroid
from astroid import nodes

from pylint.pyreverse import utils

_WrapperFuncT = Callable[[Callable[[str], nodes.Module], str], Optional[nodes.Module]]
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved


def _iface_hdlr(_):
def _iface_hdlr(_: nodes.NodeNG | Any) -> bool:
"""Handler used by interfaces to handle suspicious interface nodes."""
return True


def _astroid_wrapper(func, modname):
def _astroid_wrapper(
func: Callable[[str], nodes.Module], modname: str
) -> nodes.Module | None:
print(f"parsing {modname}...")
try:
return func(modname)
Expand All @@ -32,7 +41,11 @@ def _astroid_wrapper(func, modname):
return None


def interfaces(node, herited=True, handler_func=_iface_hdlr):
def interfaces(
node: nodes.ClassDef,
herited: bool = True,
handler_func: Callable[[nodes.NodeNG | Any], bool] = _iface_hdlr,
) -> Generator[Any, None, None]:
"""Return an iterator on interfaces implemented by the given class node."""
try:
implements = astroid.bases.Instance(node).getattr("__implements__")[0]
Expand All @@ -56,14 +69,14 @@ def interfaces(node, herited=True, handler_func=_iface_hdlr):
class IdGeneratorMixIn:
"""Mixin adding the ability to generate integer uid."""

def __init__(self, start_value=0):
def __init__(self, start_value: int = 0) -> None:
self.id_count = start_value

def init_counter(self, start_value=0):
def init_counter(self, start_value: int = 0) -> None:
"""Init the id counter."""
self.id_count = start_value

def generate_id(self):
def generate_id(self) -> int:
"""Generate a new identifier."""
self.id_count += 1
return self.id_count
Expand All @@ -72,29 +85,29 @@ def generate_id(self):
class Project:
"""A project handle a set of modules / packages."""

def __init__(self, name=""):
def __init__(self, name: str = ""):
self.name = name
self.uid = None
self.path = None
self.modules = []
self.locals = {}
self.uid: int | None = None
self.path: str = ""
self.modules: list[nodes.Module] = []
self.locals: dict[str, nodes.Module] = {}
self.__getitem__ = self.locals.__getitem__
self.__iter__ = self.locals.__iter__
self.values = self.locals.values
self.keys = self.locals.keys
self.items = self.locals.items

def add_module(self, node):
def add_module(self, node: nodes.Module) -> None:
self.locals[node.name] = node
self.modules.append(node)

def get_module(self, name):
def get_module(self, name: str) -> nodes.Module:
return self.locals[name]

def get_children(self):
def get_children(self) -> list[nodes.Module]:
return self.modules

def __repr__(self):
def __repr__(self) -> str:
return f"<Project {self.name!r} at {id(self)} ({len(self.modules)} modules)>"


Expand All @@ -121,7 +134,9 @@ class Linker(IdGeneratorMixIn, utils.LocalsVisitor):
list of implemented interface _objects_ (only on astroid.Class nodes)
"""

def __init__(self, project, inherited_interfaces=0, tag=False):
def __init__(
self, project: Project, inherited_interfaces: bool = False, tag: bool = False
) -> None:
IdGeneratorMixIn.__init__(self)
utils.LocalsVisitor.__init__(self)
# take inherited interface in consideration or not
Expand Down Expand Up @@ -180,7 +195,8 @@ def visit_classdef(self, node: nodes.ClassDef) -> None:
self.handle_assignattr_type(assignattr, node)
# resolve implemented interface
try:
node.implements = list(interfaces(node, self.inherited_interfaces))
ifaces = interfaces(node, self.inherited_interfaces)
node.implements = list(ifaces) if ifaces is not None else []
except astroid.InferenceError:
node.implements = []

Expand Down Expand Up @@ -232,7 +248,7 @@ def visit_assignname(self, node: nodes.AssignName) -> None:
frame.locals_type[node.name] = list(set(current) | utils.infer_node(node))

@staticmethod
def handle_assignattr_type(node, parent):
def handle_assignattr_type(node: nodes.AssignAttr, parent: nodes.ClassDef) -> None:
"""Handle an astroid.assignattr node.
handle instance_attrs_type
Expand Down Expand Up @@ -276,7 +292,7 @@ def visit_importfrom(self, node: nodes.ImportFrom) -> None:
if fullname != basename:
self._imported_module(node, fullname, relative)

def compute_module(self, context_name, mod_path):
def compute_module(self, context_name: str, mod_path: str) -> int:
"""Return true if the module should be added to dependencies."""
package_dir = os.path.dirname(self.project.path)
if context_name == mod_path:
Expand All @@ -285,7 +301,9 @@ def compute_module(self, context_name, mod_path):
return 1
return 0

def _imported_module(self, node, mod_path, relative):
def _imported_module(
self, node: nodes.Import | nodes.ImportFrom, mod_path: str, relative: bool
) -> None:
"""Notify an imported module, used to analyze dependencies."""
module = node.root()
context_name = module.name
Expand All @@ -301,11 +319,14 @@ def _imported_module(self, node, mod_path, relative):


def project_from_files(
files, func_wrapper=_astroid_wrapper, project_name="no name", black_list=("CVS",)
):
files: list[str],
func_wrapper: _WrapperFuncT = _astroid_wrapper,
project_name: str = "no name",
black_list: tuple[str, ...] = ("CVS",),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to make this a constant and keep it up to date with pylint's default value for the similar option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean the black_list / ignore? That would make sense imo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes!

) -> Project:
"""Return a Project from a list of files or modules."""
# build the project representation
astroid_manager = astroid.manager.AstroidManager()
astroid_manager = astroid.MANAGER
project = Project(project_name)
for something in files:
if not os.path.exists(something):
Expand Down
9 changes: 5 additions & 4 deletions pylint/pyreverse/writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import argparse
import itertools
import os
from collections.abc import Iterable

from astroid import modutils, nodes

Expand Down Expand Up @@ -56,18 +57,18 @@ def __init__(self, config: argparse.Namespace) -> None:
)
self.used_colors: dict[str, str] = {}

def write(self, diadefs):
def write(self, diadefs: Iterable[ClassDiagram | PackageDiagram]) -> None:
"""Write files for <project> according to <diadefs>."""
for diagram in diadefs:
basename = diagram.title.strip().replace(" ", "_")
file_name = f"{basename}.{self.config.output_format}"
if os.path.exists(self.config.output_directory):
file_name = os.path.join(self.config.output_directory, file_name)
self.set_printer(file_name, basename)
if diagram.TYPE == "class":
self.write_classes(diagram)
else:
if isinstance(diagram, PackageDiagram):
self.write_packages(diagram)
else:
self.write_classes(diagram)
self.save()

def write_packages(self, diagram: PackageDiagram) -> None:
Expand Down