diff --git a/src/griffe/cli.py b/src/griffe/cli.py index b2a4614a..214dd460 100644 --- a/src/griffe/cli.py +++ b/src/griffe/cli.py @@ -74,6 +74,7 @@ def _load_packages( allow_inspection: bool = True, store_source: bool = True, ) -> GriffeLoader: + # Create a single loader. loader = GriffeLoader( extensions=extensions, search_paths=search_paths, @@ -82,6 +83,8 @@ def _load_packages( allow_inspection=allow_inspection, store_source=store_source, ) + + # Load each package. for package in packages: if not package: logger.debug("Empty package name, continuing") @@ -94,6 +97,8 @@ def _load_packages( except ImportError as error: logger.exception(f"Tried but could not import package {package}: {error}") # noqa: TRY401 logger.info("Finished loading packages") + + # Resolve aliases. if resolve_aliases: logger.info("Starting alias resolution") unresolved, iterations = loader.resolve_aliases(implicit=resolve_implicit, external=resolve_external) @@ -323,6 +328,7 @@ def dump( Returns: `0` for success, `1` for failure. """ + # Prepare options. per_package_output = False if isinstance(output, str) and output.format(package="package") != output: per_package_output = True @@ -337,6 +343,7 @@ def dump( logger.exception(str(error)) # noqa: TRY401 return 1 + # Load packages. loader = _load_packages( packages, extensions=loaded_extensions, @@ -351,6 +358,7 @@ def dump( ) data_packages = loader.modules_collection.members + # Serialize and dump packages. started = datetime.now(tz=timezone.utc) if per_package_output: for package_name, data in data_packages.items(): @@ -394,6 +402,7 @@ def check( Returns: `0` for success, `1` for failure. """ + # Prepare options. search_paths = list(search_paths) if search_paths else [] try: @@ -410,6 +419,7 @@ def check( logger.exception(str(error)) # noqa: TRY401 return 1 + # Load old and new version of the package. old_package = load_git( against_path, ref=against, @@ -436,6 +446,7 @@ def check( allow_inspection=allow_inspection, ) + # Find and display API breakages. breakages = list(find_breaking_changes(old_package, new_package)) colorama.deinit() @@ -460,11 +471,13 @@ def main(args: list[str] | None = None) -> int: Returns: An exit code. """ + # Parse arguments. parser = get_parser() opts: argparse.Namespace = parser.parse_args(args) opts_dict = opts.__dict__ subcommand = opts_dict.pop("subcommand") + # Initialize logging. log_level = opts_dict.pop("log_level", DEFAULT_LOG_LEVEL) try: level = getattr(logging, log_level) @@ -478,6 +491,7 @@ def main(args: list[str] | None = None) -> int: else: logging.basicConfig(format="%(levelname)-10s %(message)s", level=level) + # Run subcommand. commands: dict[str, Callable[..., int]] = {"check": check, "dump": dump} return commands[subcommand](**opts_dict) diff --git a/src/griffe/dataclasses.py b/src/griffe/dataclasses.py index 2da559bb..42ef0dfa 100644 --- a/src/griffe/dataclasses.py +++ b/src/griffe/dataclasses.py @@ -526,22 +526,37 @@ def relative_package_filepath(self) -> Path: ValueError: When the relative path could not be computed. """ package_path = self.package.filepath + + # Current "module" is a namespace package. if isinstance(self.filepath, list): + # Current package is a namespace package. if isinstance(package_path, list): for pkg_path in package_path: for self_path in self.filepath: with suppress(ValueError): return self_path.relative_to(pkg_path.parent) + + # Current package is a regular package. + # NOTE: Technically it makes no sense to have a namespace package + # under a non-namespace one, so we should never enter this branch. else: for self_path in self.filepath: with suppress(ValueError): return self_path.relative_to(package_path.parent.parent) raise ValueError + + # Current package is a namespace package, + # and current module is a regular module or package. if isinstance(package_path, list): for pkg_path in package_path: with suppress(ValueError): return self.filepath.relative_to(pkg_path.parent) raise ValueError + + # Current package is a regular package, + # and current module is a regular module or package, + # try to compute the path relative to the parent folder + # of the package (search path). return self.filepath.relative_to(package_path.parent.parent) @property @@ -643,17 +658,27 @@ def resolve(self, name: str) -> str: Returns: The resolved name. """ + # Name is a member this object. if name in self.members: if self.members[name].is_alias: return self.members[name].target_path # type: ignore[union-attr] return self.members[name].path + + # Name was imported. if name in self.imports: return self.imports[name] + + # Name unknown and no more parent scope. if self.parent is None: # could be a built-in raise NameResolutionError(f"{name} could not be resolved in the scope of {self.path}") + + # Name is parent, non-module object. + # NOTE: possibly useless branch. if name == self.parent.name and not self.parent.is_module: return self.parent.path + + # Recurse in parent. return self.parent.resolve(name) def as_dict(self, *, full: bool = False, **kwargs: Any) -> dict[str, Any]: @@ -780,6 +805,8 @@ def __len__(self) -> int: return 1 # SPECIAL PROXIES ------------------------------- + # The following methods and properties exist on the target(s), + # but we must handle them in a special way. @property def kind(self) -> Kind: @@ -790,16 +817,6 @@ def kind(self) -> Kind: except (AliasResolutionError, CyclicAliasError): return Kind.ALIAS - @property - def lineno(self) -> int | None: - """The target lineno or the alias lineno.""" - return self.final_target.lineno - - @property - def endlineno(self) -> int | None: - """The target endlineno or the alias endlineno.""" - return self.final_target.endlineno - @property def has_docstring(self) -> bool: """Whether this alias' target has a non-empty docstring.""" @@ -837,7 +854,46 @@ def modules_collection(self) -> ModulesCollection: # no need to forward to the target return self.parent.modules_collection # type: ignore[union-attr] # we assume there's always a parent + @cached_property + def members(self) -> dict[str, Object | Alias]: # noqa: D102 + final_target = self.final_target + + # We recreate aliases to maintain a correct hierarchy, + # and therefore correct paths. The path of an alias member + # should be the path of the alias plus the member's name, + # not the original member's path. + return { + name: Alias(name, target=member, parent=self, inherited=False) + for name, member in final_target.members.items() + } + + @cached_property + def inherited_members(self) -> dict[str, Alias]: # noqa: D102 + final_target = self.final_target + + # We recreate aliases to maintain a correct hierarchy, + # and therefore correct paths. The path of an alias member + # should be the path of the alias plus the member's name, + # not the original member's path. + return { + name: Alias(name, target=member, parent=self, inherited=True) + for name, member in final_target.inherited_members.items() + } + # GENERIC OBJECT PROXIES -------------------------------- + # The following methods and properties exist on the target(s). + # We first try to reach the final target, trigerring alias resolution errors + # and cyclic aliases errors early. We avoid recursing in the alias chain. + + @property + def lineno(self) -> int | None: + """The target lineno or the alias lineno.""" + return self.final_target.lineno + + @property + def endlineno(self) -> int | None: + """The target endlineno or the alias endlineno.""" + return self.final_target.endlineno @property def docstring(self) -> Docstring | None: # noqa: D102 @@ -847,14 +903,6 @@ def docstring(self) -> Docstring | None: # noqa: D102 def docstring(self, docstring: Docstring | None) -> None: self.final_target.docstring = docstring - @cached_property - def members(self) -> dict[str, Object | Alias]: # noqa: D102 - final_target = self.final_target - return { - name: Alias(name, target=member, parent=self, inherited=False) - for name, member in final_target.members.items() - } - @property def labels(self) -> set[str]: # noqa: D102 return self.final_target.labels @@ -874,14 +922,6 @@ def aliases(self) -> dict[str, Alias]: # noqa: D102 def member_is_exported(self, member: Object | Alias, *, explicitely: bool = True) -> bool: # noqa: D102 return self.final_target.member_is_exported(member, explicitely=explicitely) - @cached_property - def inherited_members(self) -> dict[str, Alias]: # noqa: D102 - final_target = self.final_target - return { - name: Alias(name, target=member, parent=self, inherited=True) - for name, member in final_target.inherited_members.items() - } - def is_kind(self, kind: str | Kind | set[str | Kind]) -> bool: # noqa: D102 return self.final_target.is_kind(kind) @@ -952,6 +992,9 @@ def del_member(self, key: str | Sequence[str]) -> None: # noqa: D102 return self.final_target.del_member(key) # SPECIFIC MODULE/CLASS/FUNCTION/ATTRIBUTE PROXIES --------------- + # These methods and properties exist on targets of specific kind. + # We have to call the method/property on the first target, not the final one + # (which could be of another kind). @property def _filepath(self) -> Path | list[Path] | None: @@ -1013,6 +1056,8 @@ def mro(self) -> list[Class]: # noqa: D102 return cast(Class, self.final_target).mro() # SPECIFIC ALIAS METHOD AND PROPERTIES ----------------- + # These methods and properties do not exist on targets, + # they are specific to aliases. @property def target(self) -> Object | Alias: @@ -1040,6 +1085,12 @@ def final_target(self) -> Object: This will iterate through the targets until a non-alias object is found. """ + # Here we quickly iterate on the alias chain, + # remembering which path we've seen already to detect cycles. + + # The cycle detection is needed because alias chains can be created + # as already resolved, and can contain cycles. + # using a dict as an ordered set paths_seen: dict[str, None] = {} target = self @@ -1061,6 +1112,16 @@ def resolve_target(self) -> None: and added to the collection. CyclicAliasError: When the resolved target is the alias itself. """ + # Here we try to resolve the whole alias chain recursively. + # We detect cycles by setting a "passed through" state variable + # on each alias as we pass through it. Passing a second time + # through an alias will raise a CyclicAliasError. + + # If a single link of the chain cannot be resolved, + # the whole chain stays unresolved. This prevents + # bad surprises later, in code that checks if + # an alias is resolved by checking only + # the first link of the chain. if self._passed_through: raise CyclicAliasError([self.target_path]) self._passed_through = True diff --git a/src/griffe/diff.py b/src/griffe/diff.py index 293883b6..7521f901 100644 --- a/src/griffe/diff.py +++ b/src/griffe/diff.py @@ -95,6 +95,9 @@ def _relative_package_filepath(self) -> Path: @property def _location(self) -> Path: + # Absolute file path probably means temporary worktree. + # We use our worktree prefix to remove some components + # of the path on the left (`/tmp/griffe-worktree-*/griffe_*/repo`). if self._relative_filepath.is_absolute(): parts = self._relative_filepath.parts for index, part in enumerate(parts): @@ -318,7 +321,7 @@ def _format_new_value(self) -> str: return "[" + ", ".join(base.canonical_path for base in self.new_value) + "]" -# TODO: decorators! +# TODO: Check decorators? Maybe resolved by extensions and/or dynamic analysis. def _class_incompatibilities( old_class: Class, new_class: Class, @@ -327,16 +330,12 @@ def _class_incompatibilities( seen_paths: set[str], ) -> Iterable[Breakage]: yield from () - if new_class.bases != old_class.bases: - if len(new_class.bases) < len(old_class.bases): - yield ClassRemovedBaseBreakage(new_class, old_class.bases, new_class.bases) - else: - # TODO: check mro - ... + if new_class.bases != old_class.bases and len(new_class.bases) < len(old_class.bases): + yield ClassRemovedBaseBreakage(new_class, old_class.bases, new_class.bases) yield from _member_incompatibilities(old_class, new_class, ignore_private=ignore_private, seen_paths=seen_paths) -# TODO: decorators! +# TODO: Check decorators? Maybe resolved by extensions and/or dynamic analysis. def _function_incompatibilities(old_function: Function, new_function: Function) -> Iterator[Breakage]: new_param_names = [param.name for param in new_function.parameters] param_kinds = {param.kind for param in new_function.parameters} @@ -344,7 +343,7 @@ def _function_incompatibilities(old_function: Function, new_function: Function) has_variadic_kwargs = ParameterKind.var_keyword in param_kinds for old_index, old_param in enumerate(old_function.parameters): - # checking if parameter was removed + # Check if the parameter was removed. if old_param.name not in new_function.parameters: swallowed = ( (old_param.kind is ParameterKind.keyword_only and has_variadic_kwargs) @@ -355,19 +354,19 @@ def _function_incompatibilities(old_function: Function, new_function: Function) yield ParameterRemovedBreakage(new_function, old_param, None) continue - # checking if parameter became required + # Check if the parameter became required. new_param = new_function.parameters[old_param.name] if new_param.required and not old_param.required: yield ParameterChangedRequiredBreakage(new_function, old_param, new_param) - # checking if parameter was moved + # Check if the parameter was moved. if old_param.kind in POSITIONAL and new_param.kind in POSITIONAL: new_index = new_param_names.index(old_param.name) if new_index != old_index: details = f"position: from {old_index} to {new_index} ({new_index - old_index:+})" yield ParameterMovedBreakage(new_function, old_param, new_param, details=details) - # checking if parameter changed kind + # Check if the parameter changed kind. if old_param.kind is not new_param.kind: incompatible_kind = any( ( @@ -390,7 +389,7 @@ def _function_incompatibilities(old_function: Function, new_function: Function) if incompatible_kind: yield ParameterChangedKindBreakage(new_function, old_param, new_param) - # checking if parameter changed default + # Check if the parameter changed default. breakage = ParameterChangedDefaultBreakage(new_function, old_param, new_param) non_required = not old_param.required and not new_param.required non_variadic = old_param.kind not in VARIADIC and new_param.kind not in VARIADIC @@ -399,10 +398,10 @@ def _function_incompatibilities(old_function: Function, new_function: Function) if old_param.default != new_param.default: yield breakage except Exception: # noqa: BLE001 (equality checks sometimes fail, e.g. numpy arrays) - # TODO: emitting breakage on a failed comparison could be a preference + # NOTE: Emitting breakage on a failed comparison could be a preference. yield breakage - # checking if required parameters were added + # Check if required parameters were added. for new_param in new_function.parameters: if new_param.name not in old_function.parameters and new_param.required: yield ParameterAddedRequiredBreakage(new_function, None, new_param) @@ -412,7 +411,7 @@ def _function_incompatibilities(old_function: Function, new_function: Function) def _attribute_incompatibilities(old_attribute: Attribute, new_attribute: Attribute) -> Iterable[Breakage]: - # TODO: use beartype.peps.resolve_pep563 and beartype.door.is_subhint? + # TODO: Use beartype.peps.resolve_pep563 and beartype.door.is_subhint? # if old_attribute.annotation is not None and new_attribute.annotation is not None: # if not is_subhint(new_attribute.annotation, old_attribute.annotation): if old_attribute.value != new_attribute.value: @@ -500,17 +499,23 @@ def _type_based_yield( def _returns_are_compatible(old_function: Function, new_function: Function) -> bool: + # We consider that a return value of `None` only is not a strong contract, + # it just means that the function returns nothing. We don't expect users + # to be asserting that the return value is `None`. + # Therefore we don't consider it a breakage if the return changes from `None` + # to something else: the function just gained a return value. if old_function.returns is None: return True + if new_function.returns is None: - # TODO: it should be configurable to allow/disallow removing a return type + # NOTE: Should it be configurable to allow/disallow removing a return type? return False with contextlib.suppress(AttributeError): if new_function.returns == old_function.returns: return True - # TODO: use beartype.peps.resolve_pep563 and beartype.door.is_subhint? + # TODO: Use beartype.peps.resolve_pep563 and beartype.door.is_subhint? return True diff --git a/src/griffe/encoders.py b/src/griffe/encoders.py index 1efed11f..c2464f71 100644 --- a/src/griffe/encoders.py +++ b/src/griffe/encoders.py @@ -117,8 +117,15 @@ def _load_decorators(obj_dict: dict) -> list[Decorator]: def _load_expression(expression: dict) -> expressions.Expr: + # The expression class name is stored in the `cls` key-value. cls = getattr(expressions, expression.pop("cls")) expr = cls(**expression) + + # For attributes, we need to re-attach names (`values`) together, + # as a single linked list, from right to left: + # in `a.b.c`, `c` links to `b` which links to `a`. + # In `(a or b).c` however, `c` does not link to `(a or b)`, + # as `(a or b)` is not a name and wouldn't allow to resolve `c`. if cls is expressions.ExprAttribute: previous = None for value in expr.values: @@ -149,6 +156,9 @@ def _attach_parent_to_expr(expr: expressions.Expr | str | None, parent: Module | def _attach_parent_to_exprs(obj: Class | Function | Attribute, parent: Module | Class) -> None: + # Every name and attribute expression must be reattached + # to its parent Griffe object (using its `parent` attribute), + # to allow resolving names. if isinstance(obj, Class): if obj.docstring: _attach_parent_to_expr(obj.docstring.value, parent) @@ -256,14 +266,19 @@ def json_decoder(obj_dict: dict[str, Any]) -> dict[str, Any] | Object | Alias | Returns: An instance of a data class. """ + # Load expressions. if "cls" in obj_dict: return _load_expression(obj_dict) + + # Load objects and parameters. if "kind" in obj_dict: try: kind = Kind(obj_dict["kind"]) except ValueError: return _load_parameter(obj_dict) return _loader_map[kind](obj_dict) + + # Return dict as is. return obj_dict diff --git a/src/griffe/enumerations.py b/src/griffe/enumerations.py index 2f9c2f40..ae3870b0 100644 --- a/src/griffe/enumerations.py +++ b/src/griffe/enumerations.py @@ -6,7 +6,7 @@ class DocstringSectionKind(enum.Enum): - """The possible section kinds.""" + """Enumeration of the possible docstring section kinds.""" text = "text" """Text section.""" @@ -56,7 +56,7 @@ class ParameterKind(enum.Enum): class Kind(enum.Enum): - """Enumeration of the different objects kinds.""" + """Enumeration of the different object kinds.""" MODULE: str = "module" """Modules.""" @@ -71,7 +71,7 @@ class Kind(enum.Enum): class ExplanationStyle(enum.Enum): - """An enumeration of the possible styles for explanations.""" + """Enumeration of the possible styles for explanations.""" ONE_LINE: str = "oneline" """Explanations on one-line.""" @@ -80,7 +80,7 @@ class ExplanationStyle(enum.Enum): class BreakageKind(enum.Enum): - """An enumeration of the possible breakages.""" + """Enumeration of the possible API breakages.""" PARAMETER_MOVED: str = "Positional parameter was moved" PARAMETER_REMOVED: str = "Parameter was removed" @@ -97,7 +97,7 @@ class BreakageKind(enum.Enum): class Parser(enum.Enum): - """Enumeration for the different docstring parsers.""" + """Enumeration of the different docstring parsers.""" google = "google" """Google-style docstrings parser.""" @@ -108,7 +108,7 @@ class Parser(enum.Enum): class ObjectKind(enum.Enum): - """Enumeration for the different kinds of objects.""" + """Enumeration of the different runtime object kinds.""" MODULE: str = "module" """Modules.""" @@ -142,7 +142,7 @@ def __str__(self) -> str: class When(enum.Enum): - """This enumeration contains the different times at which an extension is used.""" + """Enumeration of the different times at which an extension is used.""" before_all: int = 1 """For each node, before the visit/inspection.""" diff --git a/src/griffe/exceptions.py b/src/griffe/exceptions.py index 2d6ddb38..41dff803 100644 --- a/src/griffe/exceptions.py +++ b/src/griffe/exceptions.py @@ -39,6 +39,7 @@ def __init__(self, alias: Alias) -> None: """ self.alias: Alias = alias """The alias that triggered the error.""" + message = f"Could not resolve alias {alias.path} pointing at {alias.target_path}" try: filepath = alias.parent.relative_filepath # type: ignore[union-attr] @@ -60,6 +61,7 @@ def __init__(self, chain: list[str]) -> None: """ self.chain: list[str] = chain """The chain of aliases that created the cycle.""" + super().__init__("Cyclic aliases detected:\n " + "\n ".join(self.chain)) diff --git a/src/griffe/expressions.py b/src/griffe/expressions.py index 20c3b22b..351c5b47 100644 --- a/src/griffe/expressions.py +++ b/src/griffe/expressions.py @@ -72,7 +72,7 @@ def _expr_as_dict(expression: Expr, **kwargs: Any) -> dict[str, Any]: return fields -# TODO: merge in decorators once Python 3.9 is dropped +# TODO: Merge in decorators once Python 3.9 is dropped. dataclass_opts: dict[str, bool] = {} if sys.version_info >= (3, 10): dataclass_opts["slots"] = True @@ -876,7 +876,7 @@ def _build_keyword(node: ast.keyword, parent: Module | Class, **kwargs: Any) -> def _build_lambda(node: ast.Lambda, parent: Module | Class, **kwargs: Any) -> Expr: - # TODO: better parameter handling + # FIXME: This needs better handling (all parameter kinds). return ExprLambda( [ExprParameter(ParameterKind.positional_or_keyword.value, arg.arg) for arg in node.args.args], _build(node.body, parent, **kwargs), diff --git a/src/griffe/finder.py b/src/griffe/finder.py index 40af8a9f..cb6e20a9 100644 --- a/src/griffe/finder.py +++ b/src/griffe/finder.py @@ -1,6 +1,6 @@ """This module contains the code allowing to find modules.""" -# NOTE: it might be possible to replace a good part of this module's logic +# NOTE: It might be possible to replace a good part of this module's logic # with utilities from `importlib` (however the util in question is private): # >>> from importlib.util import _find_spec # >>> _find_spec("griffe.agents", _find_spec("griffe", None).submodule_search_locations) @@ -92,7 +92,7 @@ def __init__(self, search_paths: Sequence[str | Path] | None = None) -> None: search_paths: Optional paths to search into. """ self._paths_contents: dict[Path, list[Path]] = {} - # optimization: pre-compute Paths to relieve CPU when joining paths + # Optimization: pre-compute Paths to relieve CPU when joining paths. self.search_paths = [path if isinstance(path, Path) else Path(path) for path in search_paths or sys.path] """The finder search paths.""" self._extend_from_pth_files() @@ -162,7 +162,9 @@ def find_package(self, module_name: str) -> Package | NamespacePackage: """ filepaths = [ Path(module_name), - # TODO: handle .py[cod] and .so files? + # TODO: Handle .py[cod] and .so files? + # This would be needed for package that are composed + # solely of a file with such an extension. Path(f"{module_name}.py"), ] @@ -216,9 +218,8 @@ def iter_submodules( if path.stem == "__init__": path = path.parent - # optimization: just check if the file name ends with .py[icod]/.so - # (to distinguish it from a directory), - # not if it's an actual file + # Optimization: just check if the file name ends with .py[icod]/.so + # (to distinguish it from a directory), not if it's an actual file. elif path.suffix in self.extensions_set: return @@ -234,10 +235,9 @@ def iter_submodules( # .py[cod] and .so files look like `name.cpython-38-x86_64-linux-gnu.ext` stem = stem.split(".", 1)[0] if stem == "__init__": - # optimization: since it's a relative path, - # if it has only one part and is named __init__, - # it means it's the starting path - # (no need to compare it against starting path) + # Optimization: since it's a relative path, if it has only one part + # and is named __init__, it means it's the starting path + # (no need to compare it against starting path). if len(rel_subpath.parts) == 1: continue yield rel_subpath.parts[:-1], subpath @@ -295,23 +295,23 @@ def _extend_from_pth_files(self) -> None: def _filter_py_modules(self, path: Path) -> Iterator[Path]: for root, dirs, files in os.walk(path, topdown=True): - # optimization: modify dirs in-place to exclude __pycache__ directories + # Optimization: modify dirs in-place to exclude `__pycache__` directories. dirs[:] = [dir for dir in dirs if dir != "__pycache__"] for relfile in files: if os.path.splitext(relfile)[1] in self.extensions_set: yield Path(root, relfile) def _top_module_name(self, path: Path) -> str: - # first find if a parent is in search paths + # First find if a parent is in search paths. parent_path = path if path.is_dir() else path.parent for search_path in self.search_paths: with suppress(ValueError): - # TODO: it does not work when parent_path is relative and search_path absolute + # FIXME: It does not work when `parent_path` is relative and `search_path` absolute. rel_path = parent_path.relative_to(search_path) top_path = search_path / rel_path.parts[0] return top_path.name - # if not, get the highest directory with an __init__ module, - # add its parent to search paths and return it + # If not, get the highest directory with an `__init__` module, + # add its parent to search paths and return it. while parent_path.parent != parent_path and (parent_path.parent / "__init__.py").exists(): parent_path = parent_path.parent self.search_paths.insert(0, parent_path.parent) @@ -323,8 +323,8 @@ def _top_module_name(self, path: Path) -> str: _re_import_line = re.compile(r"^import[ \t]+\w+$") -# TODO: for better robustness, we should load and minify the AST -# to search for particular call statements +# TODO: For more robustness, we should load and minify the AST +# to search for particular call statements. def _is_pkg_style_namespace(init_module: Path) -> bool: code = init_module.read_text(encoding="utf8") return bool(_re_pkgresources.search(code) or _re_pkgutil.search(code)) @@ -359,10 +359,10 @@ def _handle_pth_file(path: Path) -> list[Path]: def _handle_editable_module(path: Path) -> list[Path]: if _match_pattern(path.name, (*_editable_editables_patterns, *_editable_scikit_build_core_patterns)): - # support for how 'editables' write these files: - # example line: F.map_module('griffe', '/media/data/dev/griffe/src/griffe/__init__.py') - # and how 'scikit-build-core' writes these files: - # example line: install({'griffe': '/media/data/dev/griffe/src/griffe/__init__.py'}, {'cmake_example': ...}, None, False, True) + # Support for how 'editables' write these files: + # example line: `F.map_module('griffe', '/media/data/dev/griffe/src/griffe/__init__.py')`. + # And how 'scikit-build-core' writes these files: + # example line: `install({'griffe': '/media/data/dev/griffe/src/griffe/__init__.py'}, {'cmake_example': ...}, None, False, True)`. try: editable_lines = path.read_text(encoding="utf8").strip().splitlines(keepends=False) except FileNotFoundError as error: @@ -372,8 +372,8 @@ def _handle_editable_module(path: Path) -> list[Path]: return [new_path.parent.parent] return [new_path] if _match_pattern(path.name, _editable_setuptools_patterns): - # support for how 'setuptools' writes these files: - # example line: MAPPING = {'griffe': '/media/data/dev/griffe/src/griffe', 'briffe': '/media/data/dev/griffe/src/briffe'} + # Support for how 'setuptools' writes these files: + # example line: `MAPPING = {'griffe': '/media/data/dev/griffe/src/griffe', 'briffe': '/media/data/dev/griffe/src/briffe'}`. parsed_module = ast.parse(path.read_text()) for node in parsed_module.body: if ( diff --git a/src/griffe/loader.py b/src/griffe/loader.py index 4ca85e5f..5e20f8b5 100644 --- a/src/griffe/loader.py +++ b/src/griffe/loader.py @@ -161,10 +161,15 @@ def resolve_aliases( unresolved: set[str] = set("0") # init to enter loop iteration = 0 collection = self.modules_collection.members + + # We must first expand exports (`__all__` values), + # then expand wildcard imports (`from ... import *`), + # and then only we can start resolving aliases. for exports_module in list(collection.values()): self.expand_exports(exports_module) for wildcards_module in list(collection.values()): self.expand_wildcards(wildcards_module, external=external) + load_failures: set[str] = set() while unresolved and unresolved != prev_unresolved and iteration < max_iterations: # type: ignore[operator] prev_unresolved = unresolved - {"0"} @@ -187,7 +192,7 @@ def resolve_aliases( return unresolved, iteration def expand_exports(self, module: Module, seen: set | None = None) -> None: - """Expand exports: try to recursively expand all module exports. + """Expand exports: try to recursively expand all module exports (`__all__` values). Parameters: module: The module to recurse on. @@ -199,6 +204,8 @@ def expand_exports(self, module: Module, seen: set | None = None) -> None: return expanded = set() for export in module.exports: + # It's a name: we resolve it, get the module it comes from, + # recurse into it, and add its exports to the current ones. if isinstance(export, ExprName): module_path = export.canonical_path.rsplit(".", 1)[0] # remove trailing .__all__ try: @@ -212,6 +219,7 @@ def expand_exports(self, module: Module, seen: set | None = None) -> None: expanded |= next_module.exports except TypeError: logger.warning(f"Unsupported item in {module.path}.__all__: {export} (use strings only)") + # It's a string, simply add it to the current exports. else: expanded.add(export) module.exports = expanded @@ -235,10 +243,15 @@ def expand_wildcards( seen = seen or set() seen.add(obj.path) + # First we expand wildcard imports and store the objects in a temporary `expanded` variable, + # while also keeping track of the members representing wildcard import, to remove them later. for member in obj.members.values(): + # Handle a wildcard. if member.is_alias and member.wildcard: # type: ignore[union-attr] # we know it's an alias package = member.wildcard.split(".", 1)[0] # type: ignore[union-attr] not_loaded = obj.package.path != package and package not in self.modules_collection + + # Try loading the (unknown) package containing the wildcard importe module (if allowed to). if not_loaded: if not external: continue @@ -247,6 +260,8 @@ def expand_wildcards( except ImportError as error: logger.debug(f"Could not expand wildcard import {member.name} in {obj.path}: {error}") continue + + # Try getting the module from which every public object is imported. try: target = self.modules_collection.get_member(member.target_path) # type: ignore[union-attr] except KeyError: @@ -255,28 +270,47 @@ def expand_wildcards( f"{cast(Alias, member).target_path} not found in modules collection", ) continue + + # Recurse into this module, expanding wildcards there before collecting everything. if target.path not in seen: try: self.expand_wildcards(target, external=external, seen=seen) except (AliasResolutionError, CyclicAliasError) as error: logger.debug(f"Could not expand wildcard import {member.name} in {obj.path}: {error}") continue + + # Collect every imported object. expanded.extend(self._expand_wildcard(member)) # type: ignore[arg-type] to_remove.append(member.name) + + # Recurse in unseen submodules. elif not member.is_alias and member.is_module and member.path not in seen: self.expand_wildcards(member, external=external, seen=seen) # type: ignore[arg-type] + # Then we remove the members representing wildcard imports. for name in to_remove: obj.del_member(name) + # Finally we process the collected objects. for new_member, alias_lineno, alias_endlineno in expanded: overwrite = False already_present = new_member.name in obj.members self_alias = new_member.is_alias and cast(Alias, new_member).target_path == f"{obj.path}.{new_member.name}" + + # If a member with the same name is already present in the current object, + # we only overwrite it if the alias is imported lower in the module + # (meaning that the alias takes precedence at runtime). if already_present: old_member = obj.get_member(new_member.name) old_lineno = old_member.alias_lineno if old_member.is_alias else old_member.lineno overwrite = alias_lineno > (old_lineno or 0) # type: ignore[operator] + + # 1. If the expanded member is an alias with a target path equal to its own path, we stop. + # This situation can arise because of Griffe's mishandling of (abusive) wildcard imports. + # We have yet to check how Python handles this itself, or if there's an algorithm + # that we could follow to untangle back-and-forth wildcard imports. + # 2. If the expanded member was already present and we decided not to overwrite it, we stop. + # 3. Otherwise we proceed further. if not self_alias and (not already_present or overwrite): alias = Alias( new_member.name, @@ -285,6 +319,10 @@ def expand_wildcards( endlineno=alias_endlineno, parent=obj, # type: ignore[arg-type] ) + # Special case: we avoid overwriting a submodule with an alias pointing to it. + # Griffe suffers from this design flaw where an object cannot store both + # a submodule and a member of the same name, while this poses no issue in Python. + # We at least prevent this case where a submodule is overwritten by an imported version of itself. if already_present: prev_member = obj.get_member(new_member.name) with suppress(AliasResolutionError, CyclicAliasError): @@ -292,9 +330,10 @@ def expand_wildcards( if prev_member.is_alias: prev_member = prev_member.final_target if alias.final_target is prev_member: - # alias named after the module it targets: - # skip to avoid cyclic aliases + # Alias named after the module it targets: skip to avoid cyclic aliases. continue + + # Everything went right (supposedly), we add the alias as a member of the current object. obj.set_member(new_member.name, alias) def resolve_module_aliases( @@ -326,11 +365,16 @@ def resolve_module_aliases( seen.add(obj.path) for member in obj.members.values(): + # Handle aliases. if member.is_alias: if member.wildcard or member.resolved: # type: ignore[union-attr] continue if not implicit and not member.is_explicitely_exported: continue + + # Try resolving the alias. If it fails, check if it is because it comes + # from an external package, and decide if we should load that package + # to allow the alias to be resolved at the next iteration (maybe). try: member.resolve_target() # type: ignore[union-attr] except AliasResolutionError as error: @@ -355,6 +399,8 @@ def resolve_module_aliases( else: logger.debug(f"Alias {member.path} was resolved to {member.final_target.path}") # type: ignore[union-attr] resolved.add(member.path) + + # Recurse into unseen modules and classes. elif member.kind in {Kind.MODULE, Kind.CLASS} and member.path not in seen: sub_resolved, sub_unresolved = self.resolve_module_aliases( member, @@ -440,8 +486,7 @@ def _load_submodule(self, module: Module, subparts: tuple[str, ...], subpath: Pa try: parent_module = self._get_or_create_parent_module(module, subparts, subpath) except UnimportableModuleError as error: - # TODO: maybe add option to still load them - # TODO: maybe increase level to WARNING + # TODO: Maybe add option to still load them. logger.debug(f"{error}. Missing __init__ module?") return submodule_name = subparts[-1] diff --git a/src/griffe/logger.py b/src/griffe/logger.py index 912cd996..cd0ac17a 100644 --- a/src/griffe/logger.py +++ b/src/griffe/logger.py @@ -53,21 +53,21 @@ class _Logger: _instances: ClassVar[dict[str, _Logger]] = {} def __init__(self, name: str) -> None: - # default logger that can be patched by third-party + # Default logger that can be patched by third-party. self._logger = self.__class__._default_logger(name) - # register instance + # Register instance. self._instances[name] = self def __getattr__(self, name: str) -> Any: - # forward everything to the logger + # Forward everything to the logger. return getattr(self._logger, name) @classmethod def _patch_loggers(cls, get_logger_func: Callable) -> None: - # patch current instances + # Patch current instances. for name, instance in cls._instances.items(): instance._logger = get_logger_func(name) - # future instances will be patched as well + # Future instances will be patched as well. cls._default_logger = get_logger_func diff --git a/src/griffe/mixins.py b/src/griffe/mixins.py index 23e9ea6e..9fb41f69 100644 --- a/src/griffe/mixins.py +++ b/src/griffe/mixins.py @@ -184,9 +184,9 @@ def set_member(self, key: str | Sequence[str], value: Object | Alias) -> None: if name in self.members: # type: ignore[attr-defined] member = self.members[name] # type: ignore[attr-defined] if not member.is_alias: - # when reassigning a module to an existing one, + # When reassigning a module to an existing one, # try to merge them as one regular and one stubs module - # (implicit support for .pyi modules) + # (implicit support for .pyi modules). if member.is_module and not (member.is_namespace_package or member.is_namespace_subpackage): with suppress(AliasResolutionError, CyclicAliasError): if value.is_module and value.filepath != member.filepath: diff --git a/src/griffe/tests.py b/src/griffe/tests.py index 6e4cd62f..28a1ca71 100644 --- a/src/griffe/tests.py +++ b/src/griffe/tests.py @@ -4,8 +4,8 @@ import sys import tempfile -from collections import namedtuple from contextlib import contextmanager +from dataclasses import dataclass from importlib import invalidate_caches from pathlib import Path from textwrap import dedent @@ -24,7 +24,16 @@ TMPDIR_PREFIX = "griffe_" -TmpPackage = namedtuple("TmpPackage", "tmpdir name path") # noqa: PYI024 +@dataclass +class TmpPackage: + """A temporary package.""" + + tmpdir: Path + """The temporary directory containing the package.""" + name: str + """The package name, as to dynamically import it.""" + path: Path + """The package path.""" @contextmanager @@ -66,11 +75,7 @@ def temporary_pypackage( init: Whether to create an `__init__` module in the leaf package. Yields: - A named tuple with the following fields: - - - `tmp_dir`: The temporary directory containing the package. - - `name`: The package name, as to dynamically import it. - - `path`: The package path. + A temporary package. """ modules = modules or {} if isinstance(modules, list):