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 private API documentation #2060
base: main
Are you sure you want to change the base?
Conversation
piptools/resolver.py
Outdated
@@ -161,7 +161,9 @@ def resolve(self, max_rounds: int) -> set[InstallRequirement]: | |||
def resolve_hashes( | |||
self, ireqs: set[InstallRequirement] | |||
) -> dict[InstallRequirement, set[str]]: | |||
"""Find acceptable hashes for all of the given ``InstallRequirement``s.""" | |||
""" | |||
Find acceptable hashes for all of the given ``InstallRequirement``'s. |
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.
This change is incorrect. It's not 's
but just a plural s
. Just revert it.
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.
I wouldn't have added that if Sphinx hadn't given me this strange error:
/home/user/repos/github/pip-tools/.tox/build-docs/lib/python3.9/site-packages/piptools/resolver.py:docstring of piptools.resolver.BaseResolver.resolve_hashes:1: WARNING: Inline literal start-string without end-string.
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.
Either way, the grammar is broken. RST parser can be tricked into merging parts by adding an escaped whitespace:
Find acceptable hashes for all of the given ``InstallRequirement``'s. | |
Find acceptable hashes for all of the given ``InstallRequirement``\ s. |
If that doesn't work, the only thing left is rephrasing. There are other hacks, I think, but they are rather complicated and not worth it.
piptools/sync.py
Outdated
Calculate the dependency tree for the package `root_key` and return | ||
a collection of all its dependencies. Uses a DFS traversal algorithm. | ||
Calculate the dependency tree for the package ``root_key`` and return | ||
a collection of all its dependencies. Uses a DFS traversal algorithm. |
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.
Many people have a habit of placing two spaces in between sentences. It's an old typographic practice. I think it's also mentioned in some PEPs. Since we don't enforce one way or the other (and RST is fine with either, I think), I wouldn't attempt reformatting within this PR (maybe ever).
But talking about PEP 257-compliant docstrings, they should have exactly one title/summary sentence on a separated line. That's something that would be related.
I'd rename the PR title from "API" to "private API" for clarity. |
tox.ini
Outdated
commands_pre = | ||
commands = python -m piptools compile --strip-extras --allow-unsafe --quiet docs/requirements.in {posargs} |
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.
I'd leave this out of this PR. It's not related to documenting things.
663b618
to
7cd0ce2
Compare
7cd0ce2
to
65913a1
Compare
I've pushed a commit following a valiant attempt to format |
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
65913a1
to
cb783cc
Compare
@@ -58,3 +62,27 @@ | |||
] | |||
|
|||
suppress_warnings = ["myst.xref_missing"] | |||
|
|||
nitpick_ignore_regex = [ |
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.
Let's put this right under nitpicky
above.
docs/conf.py
Outdated
apidoc_module_dir = "../piptools" | ||
apidoc_module_first = False |
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.
Sorting
apidoc_module_dir = "../piptools" | |
apidoc_module_first = False | |
apidoc_module_first = False | |
apidoc_module_dir = "../piptools" |
piptools/resolver.py
Outdated
@@ -182,6 +184,12 @@ def _filter_out_unsafe_constraints( | |||
|
|||
|
|||
class LegacyResolver(BaseResolver): | |||
""" | |||
Resolve a given set of constraints (a collection of |
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.
It's a class, not a callable function. Classes represent static things/data. They don't do anything on their own. This is why, unlike functions/methods, their docstrings shouldn't start with a verb, because they don't perform actions, they just organize stuff. I usually recommend just stating what it is/represents in the docstring title line.
@@ -193,11 +201,6 @@ def __init__( | |||
allow_unsafe: bool = False, | |||
unsafe_packages: set[str] | None = None, | |||
) -> None: | |||
""" | |||
This class resolves a given set of constraints (a collection of |
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.
A docstring here may still be useful. It could describe what this initializer method does.
piptools/resolver.py
Outdated
@@ -416,6 +423,8 @@ def _iter_dependencies( | |||
self, ireq: InstallRequirement | |||
) -> Iterator[InstallRequirement]: | |||
""" | |||
Collect all secondary dependencies for an ireq. |
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.
I usually go for "emit" in generator functions' docstrings. I feel like this word represents better that it can produce/stream more data, than just a single return
instruction does.
Collect all secondary dependencies for an ireq. | |
Emit all secondary dependencies for an ireq. |
piptools/resolver.py
Outdated
Return true on successful resolution, otherwise remove problematic | ||
Actual resolution process. | ||
|
||
Return True on successful resolution, otherwise remove problematic |
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.
Things like this are linkable
Return True on successful resolution, otherwise remove problematic | |
Return :py:data:`True` on successful resolution, otherwise remove problematic |
piptools/resolver.py
Outdated
@@ -629,7 +640,9 @@ def _do_resolve( | |||
compatible_existing_constraints: dict[str, InstallRequirement], | |||
) -> bool: | |||
""" | |||
Return true on successful resolution, otherwise remove problematic | |||
Actual resolution process. |
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.
This should still start with a verb. Although, this specific phrase seems like a comment rather than a docstring.
file, returning the ``pathlib.Path`` of that config file if specified or | ||
discovered. | ||
|
||
``None`` is returned if no such file is found. |
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.
The original phrase was fine since it's not the title line and is in the middle of the prose / details.
The role is correct: https://webknjaz.github.io/intersphinx-untangled/docs.python.org/. The problem is that intersphinx isn't integrated (which I didn't realize), meaning that Sphinx can't look up any foreign objects from the CPython docs. |
Also remove unrelated `tox.ini` change. Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
I didn't think of that, too. Now done! |
Sometimes, GitHub is drunk and messes up internal state due to its eventual consistency. It's happening around their outages, usually. It's good it caught up with the changes. Sometimes, it doesn't and a PR may remain in a weird state forever. Such cases are only fixed with a new PR. |
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
for more information, see https://pre-commit.ci
This class resolves a given set of constraints (a collection of | ||
InstallRequirement objects) by consulting the given Repository and the | ||
DependencyCache. | ||
Make sure the legacy resolver is enabled and no backtracking resolver |
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.
This new phrasing feels weird. That's not what the initializer seems to do...
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.
I have no expertise in writing docstrings for __init__
functions. What would you expect it to contain? A listing of all the parameters?
That's not what the initializer seems to do...
Well, it does raise PipToolsError
if the user didn't enable the legacy resolver, and removes an entry for the 2020 resolver from the enabled features?
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.
I have no expertise in writing docstrings for
__init__
functions. What would you expect it to contain? A listing of all the parameters?
Yes, listing :param:
, :raises:
and other entries is something that belongs here. The original docstring could partially augment the class level one.
As for the initializer docstring title, I usually go for Initialize ClassName.
— this is what the initializer (any __init__()
) does — it's called right after the constructor (__new__()
) with a pre-created object and has to prepare it and turn into the usable state.
That's not what the initializer seems to do...
Well, it does raise
PipToolsError
if the user didn't enable the legacy resolver, and removes an entry for the 2020 resolver from the enabled features?
Yeah, but exceptions are side effects for signaling errors. While the main purpose of an initializer is to set up the instance, making it ready for the later use.
""" | ||
Calculate the dependency tree for the package `root_key` and return | ||
a collection of all its dependencies. Uses a DFS traversal algorithm. | ||
"""Calculate the dependency tree for a package |
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.
"""Calculate the dependency tree for a package | |
"""Calculate the dependency tree for a package. |
file, returning the ``pathlib.Path`` of that config file if specified or | ||
discovered. | ||
|
||
``None`` is returned if no such file is found. | ||
Return :py:data:`None` if no such file is found. |
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.
It's possible to use the param list markup for this:
Return :py:data:`None` if no such file is found. | |
:returns: :py:data:`None` if no such file is found. |
Closes #1999. Thanks for providing the example, @webknjaz!
Contributor checklist
Maintainer checklist
backwards incompatible
,feature
,enhancement
,deprecation
,bug
,dependency
,docs
orskip-changelog
as they determine changelog listing.