sphinx-doc / sphinx

The Sphinx documentation generator
https://www.sphinx-doc.org/
Other
6.45k stars 2.1k forks source link

HTML5/Latex/Text writers: Trailing comma in multi-line signatures #12902

Open Level3Manatee opened 1 week ago

Level3Manatee commented 1 week ago

Describe the bug

Multi-line signatures have a comma separator even for the last parameter when outputting HTML5 (edit: and LaTeX and Text).

The offender seems to be writers/html5.py#L232

 if opt_param_left_at_level or is_required and (is_last_group or next_is_required):

which I think should be

if opt_param_left_at_level or is_required and (not is_last_group or next_is_required):

Edit: Nm, that would break the closing dd

How to Reproduce

.. cpp:function:: Foo(int bar, float baz)

with maximum_signature_line_length = 1 outputs

Foo(
    int bar,
    float baz,
)

Environment Information

Sphinx 8.0.2

Sphinx extensions

No response

Additional context

No response

Level3Manatee commented 1 week ago

Upon further investigation, Latex and text writers are affected as well

Level3Manatee commented 1 week ago

I am pretty sure the html5 writer code should read

@@ -229,9 +229,9 @@
-            if opt_param_left_at_level or is_required and (is_last_group or next_is_required):
+            if opt_param_left_at_level or is_required and (not is_last_group or next_is_required):
                 self.body.append(self.param_separator)
-                self.body.append('</dd>\n')
+            self.body.append('</dd>\n')

(and similar changes to LaTeX and Text writers), but a number of tests seem to expect a trailing comma and I don't feel comfortable touching those, as I am not very familiar with the code base.

picnixz commented 1 week ago

We decided to keep the trailing comma actually because it was easier at that time to do it that way I think.

What we could do is probably enable/disable this feature via a config value though (personally I'd prefer no trailing commas so we could just remove the trailing comma). cc @TLouf as the original author (I will take care of PEP 695 output depending on the decision)

AA-Turner commented 6 days ago

This doesn't seem to be a bug. @Level3Manatee can you confirm this is purely a stylistic request on your part?

A

TLouf commented 6 days ago

Thanks for the ping. Yes, it's slightly easier to do it that way, but more than anything this was a choice based on personal preference (see https://github.com/sphinx-doc/sphinx/pull/11011#issuecomment-1510429111). So this behaviour could be controlled by a config value, if there's agreement on adding one.

jakobandersen commented 6 days ago

Note, the original report was about a C++ declaration, where a trailing comma is not valid, as opposed to in Python. The choice of a trailing comma should therefore be the choice of the domain and not the writers.

Level3Manatee commented 6 days ago

This doesn't seem to be a bug. @Level3Manatee can you confirm this is purely a stylistic request on your part?

As @jakobandersen has already pointed out, a trailing comma is not really a stylistic choice in the C/C++ domain. I am currently documenting C++ code, which is why I tripped over this. I assumed it was a general bug because there is no trailing comma for single-line signatures.

I would personally prefer there not being a trailing comma for Python either, but I do see why one might want it there. So maybe this should be controllable through a config value that applies to domains where it would/could be valid, i.e. std and Python?

picnixz commented 5 days ago

I think it's a sound argument that this is a domain and not a writer responsibility to decide whether a trailing comma should or shouldn't be part of the signature.

As such, I suggest we have a configuration value which enables or disables the trailing comma assuming that the language supports it. The fix shouldn't be that hard since we can simply add a node attribute (the node being altered would be the paramlist node).

In addition, I may at some point have a similar logic as for PEP 695 but for C++ template functions (namely multi-lines template parameters).

TLouf commented 5 days ago

Oh I didn't know that, my bad then! If there's agreement on having a config for that, I can propose a PR some time this week. As far as I can see, domains for which a trailing comma could make sense are Python, std, and Javascript.