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 private API documentation #2060

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

chrysle
Copy link
Contributor

@chrysle chrysle commented Feb 24, 2024

Closes #1999. Thanks for providing the example, @webknjaz!

Contributor checklist
  • Included tests for the changes.
  • PR title is short, clear, and ready to be included in the user-facing changelog.
Maintainer checklist
  • Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@chrysle chrysle added docs Documentation related skip-changelog Avoid listing in changelog labels Feb 24, 2024
docs/conf.py Outdated Show resolved Hide resolved
docs/requirements.in Outdated Show resolved Hide resolved
@@ -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.
Copy link
Member

@webknjaz webknjaz Feb 25, 2024

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.

Copy link
Contributor Author

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.

Copy link
Member

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:

Suggested change
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.
Copy link
Member

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.

@webknjaz
Copy link
Member

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}
Copy link
Member

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.

@chrysle chrysle changed the title Add API documentation Add private API documentation Feb 25, 2024
@chrysle chrysle force-pushed the add-api-docs branch 2 times, most recently from 7cd0ce2 to 65913a1 Compare February 25, 2024 14:26
@chrysle
Copy link
Contributor Author

chrysle commented Feb 25, 2024

I've pushed a commit following a valiant attempt to format pip-tools's docstrings properly.

@@ -58,3 +62,27 @@
]

suppress_warnings = ["myst.xref_missing"]

nitpick_ignore_regex = [
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

@chrysle looks like you missed this

docs/conf.py Outdated Show resolved Hide resolved
piptools/resolver.py Outdated Show resolved Hide resolved
piptools/resolver.py Show resolved Hide resolved
piptools/resolver.py Outdated Show resolved Hide resolved
piptools/resolver.py Outdated Show resolved Hide resolved
piptools/resolver.py Outdated Show resolved Hide resolved
piptools/utils.py Show resolved Hide resolved
@chrysle
Copy link
Contributor Author

chrysle commented Feb 26, 2024

Addressed your suggestions, thanks! I also moved the tox session change into another PR (#2061).

Things like this are linkable

I think the :py:data: role is only suited for module-level variables? At least Sphinx gives my a corresponding error.

@webknjaz
Copy link
Member

I think the :py:data: role is only suited for module-level variables? At least Sphinx gives my a corresponding error.

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.

@chrysle
Copy link
Contributor Author

chrysle commented Mar 1, 2024

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.

I didn't think of that, too. Now done! Strangely GH doesn't update this PR after my force-push to this branch? Ah, looks like it works, now.

@webknjaz
Copy link
Member

webknjaz commented Mar 1, 2024

Strangely GH doesn't update this PR after my force-push to this branch? Ah, looks like it works, now.

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.

docs/conf.py Outdated Show resolved Hide resolved
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
Copy link
Member

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...

Copy link
Contributor Author

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?

Copy link
Member

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.

piptools/sync.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
@chrysle chrysle force-pushed the add-api-docs branch 3 times, most recently from ec56341 to 290ad20 Compare May 12, 2024 07:08
@chrysle
Copy link
Contributor Author

chrysle commented May 12, 2024

@webknjaz Addressed your comments. Unfortunately, the time-consumingly documented __init__ method of the LegacyResolver class doesn't show up in the built documentation. Do you think that could be related to sphinx-doc/sphinx#11181?

And, I was forced to add pip's IndexContent class to nitpick_ignore_regex, as Sphinx complained about a missing reference target for the same due to this inheritance:

class FileLink(Link): # type: ignore[misc]

I tried setting autodoc_inherit_docstrings to False in conf.py, but it hasn't helped.

@chrysle chrysle force-pushed the add-api-docs branch 2 times, most recently from 75ece2a to defad02 Compare May 12, 2024 13:30
@chrysle
Copy link
Contributor Author

chrysle commented May 12, 2024

@webknjaz I really screwed up the rebasing. Now there are several redundant commits. Do you have any suggestion how to get rid of them? Please feel free to push to my branch.

@webknjaz
Copy link
Member

@chrysle interactive rebase is your friend. And remember to rebase on top of upstream, not your fork. If you can't figure it out, let me know which commits you want to keep and I'll do it myself when I get to PyCon.

chrysle and others added 6 commits May 13, 2024 09:04
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Also remove unrelated `tox.ini` change.

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
@chrysle
Copy link
Contributor Author

chrysle commented May 13, 2024

Ah, finally I made it. It was a bit harder to do, as I had amended a merge commit... I ended up applying a patch. Could you review again?

@chrysle chrysle requested a review from webknjaz May 16, 2024 14:56
@chrysle
Copy link
Contributor Author

chrysle commented Jul 1, 2024

@webknjaz?

piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

webknjaz commented Jul 3, 2024

Ping we in like a week for another review.

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related skip-changelog Avoid listing in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate API spec page
2 participants