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

Fix: no trailing comma in multi-line signatures by default #12975

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

TLouf
Copy link
Contributor

@TLouf TLouf commented Oct 5, 2024

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)

Feature or Bugfix

  • Bugfix

Relates

@picnixz
Copy link
Member

picnixz commented Oct 14, 2024

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants