-
Notifications
You must be signed in to change notification settings - Fork 49
docs: add docstrings and comments to complex functions #1047
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -169,6 +169,8 @@ def _traverse_install_requirements( | |
| start_edges: list[DependencyEdge], | ||
| visited: set[str], | ||
| ) -> typing.Iterable[DependencyEdge]: | ||
| # Depth-first walk of install requirement edges, skipping nodes | ||
| # already visited to avoid loops in the dependency graph. | ||
| for edge in start_edges: | ||
| if edge.key in visited: | ||
| continue | ||
|
|
@@ -220,6 +222,9 @@ def from_dict( | |
| graph_dict: dict[str, dict[str, typing.Any]], | ||
| ) -> DependencyGraph: | ||
| graph = cls() | ||
| # Stack-based DFS to reconstruct graph from serialized dict, | ||
| # skipping nodes already visited to avoid processing shared | ||
| # dependencies twice. | ||
|
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. This feels like it could have been a docstring instead, if you want to keep it in? Othervise, also feels that it does not add much value other than describing "what" code does (and we have the code to describe what it does already :)). |
||
| stack = [ROOT] | ||
| visited = set() | ||
| while stack: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -222,7 +222,13 @@ def get_project_from_pypi( | |
| *, | ||
| override_download_url: str | None = None, | ||
| ) -> Candidates: | ||
| """Return candidates created from the project name and extras.""" | ||
| """Fetch and filter package candidates from a PyPI-compatible server. | ||
|
|
||
| Filters the project's package list by: project status, filename | ||
| validity, package type (sdist/wheel), yanked status, Python version | ||
| compatibility, and platform tags. Can substitute | ||
| ``override_download_url`` into each candidate's URL. | ||
| """ | ||
| found_candidates: set[str] = set() | ||
| ignored_candidates: set[str] = set() | ||
| logger.debug("%s: getting available versions from %s", project, sdist_server_url) | ||
|
|
@@ -806,6 +812,10 @@ def cache_key(self) -> str: | |
| raise NotImplementedError("GenericProvider does not implement caching") | ||
|
|
||
| def find_candidates(self, identifier: typing.Any) -> Candidates: | ||
| # Accepts three input formats from _version_source: | ||
|
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. How about we make this a docstring instead? |
||
| # 1. Candidate objects (used directly) | ||
| # 2. (url, Version) tuples | ||
| # 3. (url, str) tuples (version parsed via _match_function) | ||
| candidates: list[Candidate] = [] | ||
| version: Version | None | ||
| for item in self._version_source(identifier): | ||
|
|
||
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.
Not that it does any harm, but the comment describes pretty self-explanatory code, while not adding any important context/information in the same fashion the other comments do. There is no (non-obvious) "why" explained.