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

py domain: Allow to make a style for arguments of functions and methods #6990

Merged
merged 2 commits into from Mar 14, 2020

Conversation

tk0miya
Copy link
Member

@tk0miya tk0miya commented Jan 4, 2020

Feature or Bugfix

  • Feature

Purpose

Remaining tasks

  • tests
  • documentation
  • design
    • additional nodes is needed? => No.
    • style name is appropriate?

@tk0miya tk0miya added type:enhancement enhance or introduce a new feature domains:py labels Jan 4, 2020
@tk0miya tk0miya added this to the 3.0.0 milestone Jan 4, 2020
@tk0miya tk0miya force-pushed the function_signature branch 3 times, most recently from 40c828d to 485e881 Compare January 4, 2020 13:32
CHANGES Outdated Show resolved Hide resolved
sphinx/domains/python.py Outdated Show resolved Hide resolved
sphinx/domains/python.py Outdated Show resolved Hide resolved
sphinx/domains/python.py Outdated Show resolved Hide resolved
sphinx/domains/python.py Outdated Show resolved Hide resolved
sphinx/pycode/function.py Outdated Show resolved Hide resolved
sphinx/pycode/function.py Outdated Show resolved Hide resolved
sphinx/pycode/function.py Outdated Show resolved Hide resolved
@tk0miya tk0miya force-pushed the function_signature branch 2 times, most recently from 8e7484c to 03bdb25 Compare January 4, 2020 15:01
sphinx/util/inspect.py Outdated Show resolved Hide resolved
@Thom1729
Copy link

Thom1729 commented Jan 4, 2020

Would it make sense to wrap each argument in an element? Something like:

def join(*items: List, delimiter: str = ''):
<span class="argument">
    <span class="argument-sigil">*</span><span class="argument-name">items</span>
    : <span class="annotation">List</span>
</span>,
<span class="argument">
    <span class="argument-name">delimiter</span>
    : <span class="annotation">str</span>
    = <span class="default-value">''</span>
</span>

This way, you could apply CSS to the entire argument, e.g. to prefer line breaks between arguments rather than within them.

@tk0miya
Copy link
Member Author

tk0miya commented Jan 6, 2020

Now HTML writer use <em> to wrap each argument. +1 for changing it to <span> tag :-)

@jakobandersen
Copy link
Contributor

Unfortunately I have not had time to take a closer look at this, so the following may be redundant.
The need for styling declarations is there in almost every domain so it would be nice if this PR does not exclude a general solution, or reserves generic CSS class names for the Python domain. For example, a generic CSS class argument-name should be used to style across all domains and py-argument-name should be used to apply styling to Python declarations only.
As explored and discussed a bit in #4289, one approach for the styling categories is to reuse (and refine) what is already in Pygments. Note that it should be possible to set the theme for declarations independently of the Pygments theme used for code snippets.

@tk0miya tk0miya force-pushed the function_signature branch 2 times, most recently from 1f1b7d5 to a44d6f9 Compare January 8, 2020 16:28
@shimizukawa
Copy link
Member

I reviewed the doctree structure. I think it is enough to use the desc_parameter node. I don't think we need to add nodes for argument names or type names. So, LGTM.

@eric-wieser
Copy link
Contributor

It would be neat if sphinx could parse a type annotation like MyCollection[MyType] into links to both MyCollection and MyType, but I think that should be left for a separate patch.

@tk0miya
Copy link
Member Author

tk0miya commented Feb 13, 2020

The need for styling declarations is there in almost every domain so it would be nice if this PR does not exclude a general solution, or reserves generic CSS class names for the Python domain.

It is better to surround these element by domain-classed element. It helps to choose individual element by CSS selector like dl.py.function > .argument-name. I'll work for it in another PR.

@jakobandersen
Copy link
Contributor

It is better to surround these element by domain-classed element. It helps to choose individual element by CSS selector like dl.py.function > .argument-name. I'll work for it in another PR.

Good point, and nice solution. That probably also minimizes the size of the output.

sphinx/domains/python.py Outdated Show resolved Hide resolved
@tk0miya
Copy link
Member Author

tk0miya commented Feb 14, 2020

I think some discussion points are still remained.

To concentrate these points, I'm going to make another PR for signature parser based on this PR. I think it makes this PR simple.

@tk0miya tk0miya changed the base branch from master to 3.x February 22, 2020 07:37
@tk0miya tk0miya force-pushed the function_signature branch 4 times, most recently from 2f40dda to 5dabbf9 Compare February 23, 2020 16:55
@tk0miya tk0miya marked this pull request as ready for review February 23, 2020 17:06
@tk0miya
Copy link
Member Author

tk0miya commented Feb 23, 2020

Just updated. ready for review now!

@@ -174,6 +174,31 @@ class desc_content(nodes.General, nodes.Element):
"""


class desc_sig_element(nodes.inline):
Copy link
Member Author

Choose a reason for hiding this comment

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

I added desc_sig_element and children to realize pygments based inline text decorator (see #4289).
But I also added SigElementFallbackTransform to support old translators. It helps to render these nodes as simple inline node in old translators.
It means this does not break compatibility! I confirmed working fine with docxbuilder on my local.

Copy link
Member

Choose a reason for hiding this comment

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

Excellent!

sphinx/domains/python.py Outdated Show resolved Hide resolved
else:
node += nodes.Text('=' + str(param.default))
node += addnodes.desc_sig_operator('', '=')
node += nodes.inline('', param.default, classes=['default_value'])
Copy link
Member Author

Choose a reason for hiding this comment

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

I can't find proper toke name from pygments. So I used inline node for default value. Is there good idea?
https://github.com/pygments/pygments/blob/master/pygments/token.py

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea...

@tk0miya
Copy link
Member Author

tk0miya commented Mar 6, 2020

Is there any comments? If none, I'll merge this soon. The feature freeze period for 3.x will come soon.

Copy link
Member

@shimizukawa shimizukawa left a comment

Choose a reason for hiding this comment

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

LGTM!

else:
node += nodes.Text('=' + str(param.default))
node += addnodes.desc_sig_operator('', '=')
node += nodes.inline('', param.default, classes=['default_value'])
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea...

@tk0miya tk0miya merged commit 5c0d043 into sphinx-doc:3.x Mar 14, 2020
@tk0miya
Copy link
Member Author

tk0miya commented Mar 14, 2020

Merged. Thank you for reviewing!

@tk0miya tk0miya deleted the function_signature branch March 14, 2020 08:42
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
domains:py type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants