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.

Sign up for GitHub

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

Jump to bottom

Allow configuration of trailing commas in multi-line signatures #12975

Merged
merged 22 commits into from
Jan 21, 2025
Merged

Allow configuration of trailing commas in multi-line signatures #12975

merged 22 commits into from
Jan 21, 2025

Conversation

TLouf
Copy link
Contributor

@TLouf TLouf commented Oct 5, 2024 •
edited by AA-Turner
Loading

Subject: no trailing comma in multi-line signatures by default

  • C and C++ domains will never feature a trailing comma as it'd correspond to incorrect syntax.
  • Python and JS get a new config value to optionally disable the trailing comma (for now enabled by default for backwards compatibility)

Relates

@picnixz
Copy link
Member

picnixz commented Oct 14, 2024

@TLouf Can you fix the conflicts and then request me for review please?

@TLouf
Fix: no trailing comma in multi-line signatures by default
0887051 
- C and C++ domains will never feature a trailing comma as it'd correspond to incorrect syntax.
- Python and JS get a new config value to optionally disable the trailing comma
@TLouf TLouf force-pushed the fix-trailing-comma branch from a272868 to 0887051 Compare October 19, 2024 11:52
@TLouf
Copy link
Contributor Author

TLouf commented Oct 19, 2024

@picnixz Done, I think this is ready for review.

@picnixz picnixz self-requested a review October 25, 2024 09:37
picnixz
picnixz reviewed Oct 25, 2024
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Do you want to add tests for PEP 695 as well or do you want me to take care of that later (I don't know when)?

CHANGES.rst Outdated Show resolved Hide resolved
doc/usage/configuration.rst Show resolved Hide resolved
doc/usage/configuration.rst Show resolved Hide resolved
sphinx/domains/javascript.py Outdated Show resolved Hide resolved
sphinx/domains/python/__init__.py Outdated Show resolved Hide resolved
sphinx/domains/python/_object.py Outdated Show resolved Hide resolved
sphinx/domains/python/_object.py Outdated Show resolved Hide resolved
sphinx/domains/python/_object.py Outdated Show resolved Hide resolved
sphinx/domains/python/_object.py Outdated Show resolved Hide resolved
tests/test_domains/test_domain_py.py Show resolved Hide resolved
TLouf and others added 4 commits October 26, 2024 13:00
jakobandersen
jakobandersen reviewed Oct 26, 2024
View reviewed changes
CHANGES.rst Outdated Show resolved Hide resolved
@TLouf
Copy link
Contributor Author

TLouf commented Oct 26, 2024

Do you want to add tests for PEP 695 as well or do you want me to take care of that later (I don't know when)?

I've just added one for the HTML writer, but since there isn't any for the text writer I'm not sure whether to add both the base test for maximum_signature_line_length and another for the trailing comma here.

picnixz
picnixz reviewed Jan 5, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I forgot about this PR, but this looks good. Can you resolve the conflicts please? (most of the conflicts arise because we are formatting everything now so you can run the make rule to autoformat this (I don't remember which one))

@TLouf TLouf force-pushed the fix-trailing-comma branch from ce595dd to 2c1e59f Compare January 8, 2025 14:28
@TLouf
Copy link
Contributor Author

TLouf commented Jan 8, 2025

@picnixz done

@picnixz picnixz self-requested a review January 12, 2025 09:14
picnixz
picnixz approved these changes Jan 12, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

A final nit and LGTM.

CHANGES.rst Outdated Show resolved Hide resolved
tests/test_builders/test_build_latex.py Show resolved Hide resolved
@picnixz picnixz added this to the 8.2.0 milestone Jan 20, 2025
@picnixz
Copy link
Member

picnixz commented Jan 20, 2025

I'll take over your PR and will fix the conflicts. I need first to merge #13255 for the docs to build and then we'll fix the conflicts either before 8.2 release or when we have time. So no worries on this getting merged.

@AA-Turner just to remember to incorporate this into 8.2 (I've added this PR to the milestone)

@AA-Turner AA-Turner changed the title Fix: no trailing comma in multi-line signatures by default Allow users to configure trailing commas in multi-line signatures Jan 21, 2025
@AA-Turner AA-Turner changed the title Allow users to configure trailing commas in multi-line signatures Allow configuration of trailing commas in multi-line signatures Jan 21, 2025
@AA-Turner AA-Turner merged commit 1418339 into sphinx-doc:master Jan 21, 2025
22 checks passed
@AA-Turner
Copy link
Member

Thank you @TLouf!

A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakobandersen jakobandersen jakobandersen left review comments

@picnixz picnixz picnixz approved these changes

Assignees
No one assigned
Labels
builder domains:c domains:cpp domains:js domains:py
Projects
None yet
Milestone
8.2.0
Development

Successfully merging this pull request may close these issues.

HTML5/Latex/Text writers: Trailing comma in multi-line signatures
5 participants
@TLouf @picnixz @AA-Turner @jakobandersen @jayaddison