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

DOC: include positional-only and keyword-only parameters in standard #367

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

namurphy
Copy link
Contributor

@namurphy namurphy commented Mar 2, 2022

This PR adds keyword-only parameters to the numpydoc style guide, which were added in PEP 3102. This change would specify that keyword-only gets added next to the type information, like with optional. This was suggested in #330. Here are two examples:

def f(*, x, y=None):
    """
    Parameters
    ----------
    x : int, keyword-only
        ...
    y : int, keyword-only, optional
        ...
    """
    ...

Here are some alternatives that I considered:

  • Specify that a parameter is keyword-only in the description of the parameter. Some examples.
    def f(*, x=None, y=None):
        """
        Parameters
        ----------
        x : int, optional
            Some number.  Keyword-only.
        y : int, optional
            Or it could be in parentheses (keyword-only).  
        """
    I didn't choose this approach since the keyword-only might be missed by someone who is quickly scanning through the parameters, and because the information about if something is keyword-only is closely related to the information about if a parameter is optional.
  • Include a separate section for keyword-only parameters. I didn't do this because if a parameter is keyword-only and required, someone might miss it if it is in a different section.
  • Do not include keyword-only parameters in the standard. I didn't choose this approach because right now there are different practices for specifying keyword-only parameters. If someone is expecting keyword-only to show up next to the type, then they would be more likely to miss it if it is in the parameter description. Also, if someone wanted to create an automated tool to automatically link to a definition of keyword-only, having a standard to follow would make it easier to implement.

Closes #330. I'm open to alternatives and edits, in case anyone has suggestions. Many thanks!

@larsoner
Copy link
Collaborator

larsoner commented Mar 2, 2022

Sounds reasonable to me. I recently started adding keyword-only via ..., *, ... in some projects. It would be great if this were automagically documented!

@namurphy namurphy changed the title DOC: include keyword-only parameters in standard DOC: include positional-only and keyword-only parameters in standard Mar 2, 2022
@namurphy
Copy link
Contributor Author

namurphy commented Mar 2, 2022

Cool! I just edited this to include positional-only parameters also, which I had forgotten about because the main project I'm working on is currently Python 3.7+, and this capability was added in 3.8.

def f(x, /):
    """
    Parameters
    ----------
    x : int, positional-only
    """

One thing I'm not sure about is if the Python code needs to be updated to reflect these changes, but if so, I'd be happy to try to make those changes too.

@larsoner
Copy link
Collaborator

larsoner commented May 9, 2022

One thing I'm not sure about is if the Python code needs to be updated to reflect these changes, but if so, I'd be happy to try to make those changes too.

It probably does at least in terms of not hitting a problem with automatic xref resolution. It shouldn't try to resolve these new terms

@rossbar
Copy link
Contributor

rossbar commented May 9, 2022

In principle it'd be nice to add related checks to the docstring validation as well, to help codify the updates to the standard.

doc/conf.py Outdated
Comment on lines 86 to 93
numpydoc_xref_ignore = {
'optional',
'type_without_description',
'BadException',
'keyword-only',
'positional-only',
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This addition is not particularly necessary since "keyword-only" and "positional-only" don't appear in any docstrings in the repo that I could find, but I tentatively included it in here to be consistent with the standard.

@namurphy
Copy link
Contributor Author

I was thinking that "keyword-only" and "positional-only" should be treated the same as "optional", since they're used in the same place for similar reasons.

I took a look at the code and if I'm understanding things right, numpydoc_xref_ignore defaults to an empty set, so at present it is necessary to include "optional" in numpydoc_xref_ignore in order for it to be ignored.

I put "keyword-only" and "positional-only" in the example for numpydoc_xref_ignore in the configuration section, and tentatively did the same thing in doc/conf.py too.

@namurphy
Copy link
Contributor Author

In principle it'd be nice to add related checks to the docstring validation as well, to help codify the updates to the standard.

That's an interesting thought! I'd really like a tool (maybe a pre-commit hook?) that would look for positional-only, keyword-only, or optional parameters in function signature, and then make sure that the appropriate term shows up in the type description.

@namurphy
Copy link
Contributor Author

Just to follow up on this since it's been a few months, is there a timeline for deciding on this? Thanks!

@jarrodmillman jarrodmillman added this to the 1.5.0 milestone Aug 13, 2022
@larsoner
Copy link
Collaborator

@jarrodmillman marked it for the 1.5.0 milestone, so hopefully we'll discuss before then

@rossbar
Copy link
Contributor

rossbar commented Aug 16, 2022

Just to follow up on this since it's been a few months, is there a timeline for deciding on this? Thanks!

I'm generally in favor, though I do think that having an associated validation check for this is important. @namurphy I'm happy to take a stab at this, though I can't make any promises re: timeline!

@namurphy
Copy link
Contributor Author

@rossbar — please feel free to! Since you're not sure about the timeline, would it make sense to have the validation be in a separate pull request?

"Keyword-only" and "optional" behave really similarly here. I did a search for "optional" in this repo, and it didn't seem like there was a validation for "optional" either (though I'm not particularly familiar with the source code). I'm wondering if it would be helpful to include a validation for "optional" at the same time.

Thank you!

@rossbar
Copy link
Contributor

rossbar commented Aug 17, 2022

it didn't seem like there was a validation for "optional" either (though I'm not particularly familiar with the source code). I'm wondering if it would be helpful to include a validation for "optional" at the same time.

I think this is just historical - "optional" has been around since the beginning but the validation module is much newer. +1 for adding additional validation for "optional" as well, and I agree it will be very similar to the validation checks proposed here so it may make sense to add it here as well.

The reason I'm strongly advocating for adding validation checks for new additions to the standard is that we can then run the checks against some of the core ecosystem libraries (numpy, scipy, matplotlib, the scikits, etc.) to check that any new additions to the standard aren't particularly disruptive. It's a bit more work, but it will provide strong evidence in favor of adopting new standards.

@jarrodmillman jarrodmillman removed this from the 1.7.0 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convention for specifying that arguments are keyword only?
4 participants