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 support for formatting Python signatures using black #367

Merged
merged 4 commits into from
Jul 21, 2024

Conversation

jbms
Copy link
Owner

@jbms jbms commented Jul 11, 2024

No description provided.

@jbms jbms requested a review from 2bndy5 July 11, 2024 06:12
@jbms jbms unassigned 2bndy5 Jul 11, 2024
@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 11, 2024

What about ruff format? It hails itself as a drop-in replacement for black. I switched to ruff a while back and haven't looked back since. 😃

@jbms
Copy link
Owner Author

jbms commented Jul 11, 2024

Potentially ruff could be supported in addition or instead. Most of the work in this PR is related to the Python-specific munging of signatures that would be the same for ruff as well.

The advantage of black is that it has a Python API and therefore we can just format each signature individually using black.format_str. For ruff, since it has to be invoked as a subprocess, we need some mechanism of batching to avoid the overhead of starting a new process for each signature.

For clang-format we already do that, by collecting all of the signatures, separating them with special identifying comments, and formatting all of them with a single run of clang-format.

However, there are a few caveats with applying the same approach to Python:

  • black (and probably ruff also) is much less forgiving than clang-format about syntax errors. A problem with any signature would mean none get formatted. We would have to deal with this somehow.
  • With the type parameters feature, apigen can produce signatures with two type parameter lists: https://sphinx-immaterial--366.org.readthedocs.build/en/366/python_apigen_generated/Map.get.html I already have to munge the signatures to make them valid Python syntax so that black can handle them, but I couldn't think of a way to make this work in a single pass. Instead, signatures with an extra type parameter list on the parent class are handled by first formatting up to the end of the first type parameter list, then formatting the remaining part with a suitable prefix added to make the line length limit work. This could still be handled via a batch but just with two separate passes possibly being required, but it would add an extra complication.

Potentially the ruff LSP accessible via ruff serve could be used to efficiently issue a bunch of independent format requests. I'm not sure how difficult that would be.

Overall, though, I expect black will be fast enough, and other than speed I don't know that there is any real advantage to using ruff, and therefore the added hassle of integrating ruff may not be worth it.

I'd be happy to switch this project itself to using ruff instead of black/pylint, though.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 11, 2024

I'd be happy to switch this project itself to using ruff instead of black/pylint, though.

Yes, please! That should cut down the longest steps in our CI's check-convention job.


You make a good point about the lack of an API for ruff. One of the biggest unspoken hurtles with PyO3 is the compatibility with python ABI vs rust edition.

Now that you mention that this is only for signatures, I agree it may not be worth pursuing. My question was kind of a knee-jerk reaction; I recently fell in love with rust and am looking for ways to use it more and more.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 11, 2024

image

🤣 That's just damn ironic for this PR.

@jbms
Copy link
Owner Author

jbms commented Jul 11, 2024

Most of the pylint failures are related to my using list[T] in place of typing.List[T] and similar. It would certainly be possible to retain support for Python 3.8, but in the type parameter PR I already included a commit to drop Python 3.8 support, since at this point I don't think it is useful to keep support for it. Or do you have a strong desire to keep Python 3.8 support still?

@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 11, 2024

Personally, I'm ok with dropping it. However, EoL for python 3.8 is still 3 months from now, so its just a little early. IMHO, it should probably be done in a separate PR because there is likely more than just type hints in terms of code specific to python 3.8 (or earlier).

@jbms jbms force-pushed the feat-format-signatures-black branch from c862962 to 5137323 Compare July 11, 2024 20:30
@jbms jbms force-pushed the feat-format-signatures-black branch from 5137323 to 6819422 Compare July 18, 2024 22:15
Previously, signatures collected from multiple workers were not
correctly merged, leading to some signatures not being formatted in
parallel builds.
@jbms jbms force-pushed the feat-format-signatures-black branch 4 times, most recently from 5109c4f to 8f8edba Compare July 19, 2024 20:38
@jbms
Copy link
Owner Author

jbms commented Jul 19, 2024

This now passes CI, and I added some documentation.

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 7.57576% with 244 lines in your changes missing coverage. Please review.

Project coverage is 63.03%. Comparing base (7796775) to head (dd125ad).
Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
sphinx_immaterial/apidoc/format_signatures.py 0.00% 233 Missing ⚠️
sphinx_immaterial/apidoc/python/apigen.py 0.00% 7 Missing ⚠️
sphinx_immaterial/apidoc/python/object_ids.py 71.42% 2 Missing ⚠️
...hinx_immaterial/apidoc/python/parameter_objects.py 90.00% 1 Missing ⚠️
sphinx_immaterial/apidoc/python/synopses.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #367      +/-   ##
==========================================
- Coverage   63.91%   63.03%   -0.89%     
==========================================
  Files          67       67              
  Lines        8068     8207     +139     
==========================================
+ Hits         5157     5173      +16     
- Misses       2911     3034     +123     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jbms
Copy link
Owner Author

jbms commented Jul 19, 2024

This kind of works with the latex builder but the spacing doesn't line up correctly because the latex builder doesn't use a monospace font for the signatures. I'm not sure how easily we could force a monospace font.

More generally, though, while building the latex docs (i.e. actually invoke make after running sphinx) I noticed that the latex build is not in a great shape --- the table of contents seems to be broken, and we get a lot of undefined references.

2bndy5
2bndy5 previously requested changes Jul 19, 2024
sphinx_immaterial/apidoc/format_signatures.py Outdated Show resolved Hide resolved
sphinx_immaterial/apidoc/format_signatures.py Outdated Show resolved Hide resolved
sphinx_immaterial/apidoc/format_signatures.py Outdated Show resolved Hide resolved
sphinx_immaterial/apidoc/format_signatures.py Outdated Show resolved Hide resolved
sphinx_immaterial/apidoc/format_signatures.py Outdated Show resolved Hide resolved
docs/apidoc/format_signatures.rst Outdated Show resolved Hide resolved
sphinx_immaterial/apidoc/format_signatures.py Outdated Show resolved Hide resolved
@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 19, 2024

I noticed that the latex build is not in a great shape --- the table of contents seems to be broken, and we get a lot of undefined references.

Yeah, its been like that for a long time. I never devoted enough time to figure out and fix the problems. I'm happy enough just keeping the builder from breaking (for now).

@jbms jbms force-pushed the feat-format-signatures-black branch 2 times, most recently from 38bdc91 to 87d2a40 Compare July 19, 2024 23:51
@jbms jbms requested a review from 2bndy5 July 19, 2024 23:51
@2bndy5 2bndy5 dismissed their stale review July 19, 2024 23:53

resolved

sphinx_immaterial/apidoc/format_signatures.py Outdated Show resolved Hide resolved
sphinx_immaterial/apidoc/format_signatures.py Outdated Show resolved Hide resolved
@jbms jbms force-pushed the feat-format-signatures-black branch from 87d2a40 to dd125ad Compare July 20, 2024 05:17
@jbms jbms requested a review from 2bndy5 July 20, 2024 05:18
@jbms jbms merged commit 9d011ed into main Jul 21, 2024
62 checks passed
@jbms jbms deleted the feat-format-signatures-black branch July 21, 2024 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants