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

ENH: enable multiline signatures when too long. #11011

Merged
merged 110 commits into from May 11, 2023

Conversation

TLouf
Copy link
Contributor

@TLouf TLouf commented Dec 3, 2022

Subject: enable multiline signatures when they are too long.

Feature or Bugfix

  • Feature

Purpose

Improve the readability of API references by allowing to set a number of characters a (for instance) Python function's signature cannot exceed. When it would, display each parameter on its own indented line, as should be done in code following style guides like PEP 8.

Detail

  • The proposed implementation introduces a configuration option python_maximum_signature_line_length, an integer representing the maximum number of characters that cannot be exceeded by a signature. When negative (the default), there is no maximum, no line break will be introduced no matter how long the signature.
  • A :singlelinesig: directive still allows to disable this behavior on a per-object basis.

See for the example, with python_maximum_signature_line_length = 88 set in conf.py, the rst below:

.. function:: fun_with_short_sig(a, b, c)

.. function:: fun_with_long_sig(this_is_one_very_long_argument_name, what_about_this_one='and_its_default')

.. function:: another_fun_with_long_sig(this_is_one_very_long_argument_name, what_about_this_one='and_its_default')
   :singlelinesig:

would build to the following HTML:

image

  • This is still very much a work in progress, as I've only looked at Python and html, the CSS is surely not perfect and I haven't introduced new tests or modified the docs. In its current state see it more as a proof of concept.

Relates

#1514 pandas-dev/pandas#49945

@emezh
Copy link

emezh commented Dec 15, 2022

Hope this will work for C too (and not just for Py). Thanks in advance.

@TLouf
Copy link
Contributor Author

TLouf commented Dec 17, 2022

Thanks for showing your interest :) I've implemented the functionality similarly for C and C++.

If a maintainer finds the time, would be good to hear from one of them whether this seems like the way to go, or if there's something I'm not aware of that makes this implementation flawed.

@pradyunsg
Copy link
Contributor

@TLouf Thanks for this PR! I'm excited to see this moving forward! ^>^

In case you missed it, this pull request is currently failing continous integration checks since mypy's type checking does not like some of the changes in _parse_arglist.

@jakobandersen
Copy link
Contributor

From a very brief look:

  • the addnodes.desc_content() should not be used here, as it is reserved for the content of a directive (i.e., the indented stuff with description of an object).
  • it would be great if the option singlelinesig would be more general, e.g., param-line-spec where one could specify which parameters should be grouped on lines. The specification language probably requires a bit of discussion iteration. Also, see the next comment. Some simple cases could for example be
    • 1-3,4-: "let parameter 1 through 3 be on the line with the function name, and the rest on the next line"
    • ,1-: "after the opening (, put the rest on the next line"
  • is a "max length" character constraint good for HTML? When people are using different screens, different zoom levels, etc. it feels like the line breaking should be dynamical.
    Once upon a time I tried to sketch how the HTML structure and CSS could be set up, but never found time to get into a real implementation: Break long function declarations into one parameter per line #1514 (comment)
  • For the C and C++ domains (perhaps Python as well), remember to increment the environment version in the bottom when the ASTs are modified.

@pradyunsg
Copy link
Contributor

pradyunsg commented Dec 27, 2022

is a "max length" character constraint good for HTML?

Yes.

It's the same discussion as why we should have a line length/content width limit and most of the text/prose arguments for websites made in https://mediawiki.org/wiki/Reading/Web/Desktop_Improvements/Features/Limiting_content_width and we've had a similar discussion in the issue about limiting content width in the default Sphinx themes (sorry for not finding the issue number; it's 1am and a quick search didn't surface it).

@TLouf
Copy link
Contributor Author

TLouf commented Dec 29, 2022

the addnodes.desc_content() should not be used here, as it is reserved for the content of a directive (i.e., the indented stuff with description of an object).

Fixed, I introduced a new desc_parameterline class to hold parameter lines.

it would be great if the option singlelinesig would be more general, e.g., param-line-spec

This would be significantly more complex to implement I think, especially because what you describe is a completely different style: namely one with a block indented at the level of the signature's opening parenthesis. What I had in mind is a Black-like style, which has every parameter on its own line, indented by one level more than the object's name, which is straightforward to implement in HTML/CSS and other output formats with some kind of list environment. So I would maybe reserve that idea for another PR.

Otherwise, I'm a bit stuck on the LaTeX implementation now, especially for C and C++. I was thinking of putting the arguments in a fulllineitems environment, have the function name and each argument in separate pysiglines. But I can't figure out how to wrap the return type and function name of C functions in a pysigline.

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Brief review. Please add tests and documentation for the proposed feature.

A

sphinx/addnodes.py Outdated Show resolved Hide resolved
sphinx/domains/c.py Outdated Show resolved Hide resolved
sphinx/domains/c.py Outdated Show resolved Hide resolved
@AA-Turner
Copy link
Member

AA-Turner commented Apr 21, 2023

[I'm starting to work towards preparing the 6.2.0 release this weekend -- setting expectations that this PR mightn't be in 6.2]

A

@jfbu
Copy link
Contributor

jfbu commented Apr 21, 2023

Thanks for this ping @AA-Turner! As far as LaTeX is concerned it is all good for me to merge into 6.2.0, I hope my numerous comments did not create impression to contrary. Testing is fine as @TLouf has solved the issue I was not able to address properly a few weeks (or days?) ago, I will now add an additional check of mark-up, and push an update to latex.rst to mention the \sphinxparam, \sphinxparamcomma and \sphinxparamcommaoneperline that we added to help customize signatures. For LaTeX all looks good to me. I am ok with having a comma after the final parameter of a function, which is an aspect @TLouf was submitting for further discussion.

I hesitated:

- whether to reuse as here a test source which was prepared for Python
  domain and used there already,

- in place of this, rather add a check of the sphinxtests.tex contents
  as output from the test_build_latex_doc for pdflatex engine.
@jfbu jfbu mentioned this pull request Apr 21, 2023
doc/latex.rst Show resolved Hide resolved
@TLouf
Copy link
Contributor Author

TLouf commented Apr 22, 2023

@AA-Turner Just pushed tests for text output, they're very similar to the ones for HTML output so it was straightforward. From my point of view this PR is ready for a final review. If you can find the time to squeeze this one into 6.2.0, it would be a really nice addition I think :)

@AA-Turner AA-Turner modified the milestones: 6.x, 7.x Apr 29, 2023
@jfbu jfbu modified the milestones: 7.x, 7.1.0 Apr 30, 2023
@AA-Turner AA-Turner merged commit 86b07d4 into sphinx-doc:master May 11, 2023
27 checks passed
@AA-Turner
Copy link
Member

Thank you @TLouf (& @jfbu) for your work on this PR!

A

@jfbu
Copy link
Contributor

jfbu commented May 12, 2023

Thanks for updates and merge @AA-Turner! I am a bit lost again in our release policy. This was merged to master and shows in CHANGES as being part of upcoming 7.0.1. Btw there is no 7.0.x branch indeed. In the past we used patch number bump only for urgent hotfixes, which this is not.

Related to my query: in doc/latex.rst I documented on line 1482 the change to appear in 7.1.0. (sorry but in the past I never understood how to link directly to a specific line of a .rst file on github interface and I have not tried again since)

@AA-Turner
Copy link
Member

This was merged to master and shows in CHANGES as being part of upcoming 7.0.1. Btw there is no 7.0.x branch indeed.

Thank you for pointing this out, I hadn't noticed that typo (7.0.1 vs 7.1.0)! You are of course right that the unreleased section of CHANGES should read 7.1.0.

I didn't create a 7.0.x branch, on the rationale that if we needed a hot-fix it would be very simple to create the branch at a later date.

7.1.0 was also the version I bumped all the other changes to, e.g.:

.. confval:: maximum_signature_line_length
If a signature's length in characters exceeds the number set, each
parameter within the signature will be displayed on an individual logical
line.
When ``None`` (the default), there is no maximum length and the entire
signature will be displayed on a single logical line.
A 'logical line' is similar to a hard line break---builders or themes may
choose to 'soft wrap' a single logical line, and this setting does not affect
that behaviour.
Domains may provide options to suppress any hard wrapping on an individual
object directive, such as seen in the C, C++, and Python domains (e.g.
:rst:dir:`py:function:single-line-parameter-list`).
.. versionadded:: 7.1

A

@jfbu
Copy link
Contributor

jfbu commented May 12, 2023

@AA-Turner ah it was a typo! I would not have imagined that 😄, I thought to have monopoly on such things...

Sorry I had missed you bumped to 7.1 elsewhere in the doc. Quite clear now why there is no 7.0.x branch.

And finally, thanks for showing me the ?plain=1#L682-L699 syntax for rst files! That's a huge leap forward in my github mastery...

@AA-Turner
Copy link
Member

AA-Turner commented May 12, 2023

And finally, thanks for showing me the ?plain=1#L682-L699 syntax for rst files! That's a huge leap forward in my github mastery...

Appending ?plain=1 to the URL makes the file appear in the GitHub viewer like a standard code file, and then one can use the standard tools when clicking on a line number -- Copy permalink is then the one I use to have the line numbers automatically appended to the URL.

If you use the refined github browser extension, there's a button added automatically to view as 'code' or 'preview', e.g.:

image

A

@TLouf TLouf deleted the signature-linebreaks branch May 13, 2023 15:44
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants