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: fix PR02 errors in docstrings - pandas.describe_option, pandas.get_option, pandas.reset_option #57117

Merged
merged 3 commits into from
Jan 29, 2024

Conversation

jrmylow
Copy link
Contributor

@jrmylow jrmylow commented Jan 28, 2024

Test results:
Describe_option

$ scripts/validate_docstrings.py --format=actions --errors=PR02 pandas.describe_option
...
################################################################################
################################## Validation ##################################
################################################################################

1 Errors found for `pandas.describe_option`:
        SA01    See Also section not found

Get_option

$ scripts/validate_docstrings.py --format=actions --errors=PR02 pandas.get_option
...
################################################################################
################################## Validation ##################################
################################################################################

2 Errors found for `pandas.get_option`:
        PR01    Parameters {'silent'} not documented
        SA01    See Also section not found

Reset_option

$ scripts/validate_docstrings.py --format=actions --errors=PR02 pandas.reset_option
...
################################################################################
################################## Validation ##################################
################################################################################

2 Errors found for `pandas.reset_option`:
        PR01    Parameters {'silent'} not documented
        SA01    See Also section not found

@jrmylow
Copy link
Contributor Author

jrmylow commented Jan 28, 2024

Note, set_option is a bit trickier because it accepts any number of pairs of arguments, will investigate more there

@jrmylow jrmylow requested a review from mroeschke as a code owner January 28, 2024 11:28
@jrmylow jrmylow marked this pull request as draft January 28, 2024 11:30
@datapythonista
Copy link
Member

Thanks for the contribution @jrmylow. I don't understand your changes here. If you're fixing PR02 errors in docstrings, shouldn't we be adding missing parameters to the docs here? I don't know what is the change in the config module, is this intentional?

@jrmylow
Copy link
Contributor Author

jrmylow commented Jan 29, 2024

Oh right, I forgot to document the reason for the changes.

From 1, I understand that the PR01/02 errors are for function parameters that haven't been documented, and documented parameters that aren't in a function respectively.

What I found is that the way these methods are constructed means that they have a strange signature, and the use of inspect/signature is meant to rectify that.

Taking pandas.reset_option as an example, the message I get is 3 errors found:

PR01  Parameters {'*args', '**kwargs'} not documented
PR02  Unknown parameters {'pat'}
SA01 See Also section not found

reset_option is not defined directly, instead through a CallableDynamicDoc

reset_option = CallableDynamicDoc(_reset_option, _reset_option_tmpl)

The function _reset_option is defined and its signature includes the parameter pat:

def _reset_option(pat: str, silent: bool = False) -> None:

The docstring _reset_option_tmpl is also defined and references the parameter pat as str/regex:

_reset_option_tmpl = """

pat : str/regex

However, the PR02 error does not recognise that pat exists despite being in both the _reset_option signature and the docstring.

When examining the signature of pandas.reset_option using inspect.signature() I find that it is actually:

>>> inspect signature(pandas.reset_option)
<Signature (*args, **kwds) -> 'T'>

The fix applied has been to copy the function signature (in this case _reset_option and apply it to CallableDynamicDoc)

@datapythonista hope this makes sense, happy to proceed with another method if there is a strong argument against this way

@jrmylow
Copy link
Contributor Author

jrmylow commented Jan 29, 2024

Note as a follow-on from the above, the reason this does not fix set_options is that this function actually does accept any number of args and kwargs (contradicting the documentation, which only specifies set_option(pat, value)), and I haven't figured out a clean way to fix this yet.

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying @jrmylow, this wasn't immediately obvious from the diff. This seems reasonable, I guess it's the best fix we can do. Is there anything else missing here? I see this as a draft PR

@jrmylow
Copy link
Contributor Author

jrmylow commented Jan 29, 2024

This fix only fixes 3 out of 4 of the options. set_option is tricky because the underlying method signature is in fact _set_option(*args, **kwargs) and contradicts the docstring, which expects set_option(pat, value). I wanted to fix all of them in a single PR given it's the same issue and the same module.

@datapythonista
Copy link
Member

I would personally prefer if you address further fixes in new PRs. For reviewers, we'll have to keep reviewing the changes you already have at every iteration if you continue working on this PR, which is not a big problem since the changes are small, but it's still more efficient if we have to review only new changes in the future.

@jrmylow
Copy link
Contributor Author

jrmylow commented Jan 29, 2024

Ok fair, I'll un-draft it and wait for the code checks to all pass

@jrmylow jrmylow marked this pull request as ready for review January 29, 2024 05:28
@jrmylow jrmylow changed the title DOC: fix PR02 errors in docstrings - pandas.describe_option, pandas.set_option, pandas.get_option, pandas.reset_option DOC: fix PR02 errors in docstrings - pandas.describe_option, pandas.get_option, pandas.reset_option Jan 29, 2024
@mroeschke mroeschke modified the milestones: 2.2.1, 3.0 Jan 29, 2024
@mroeschke mroeschke merged commit 42e4e4c into pandas-dev:main Jan 29, 2024
48 of 50 checks passed
@mroeschke
Copy link
Member

Thanks @jrmylow

@jrmylow jrmylow deleted the DOC-PR02-pandas._config.config.py branch January 30, 2024 07:11
pmhatre1 pushed a commit to pmhatre1/pandas-pmhatre1 that referenced this pull request May 7, 2024
…et_option, pandas.reset_option (pandas-dev#57117)

* Copy the signature from the implementation

* updated code_checs.sh
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.

3 participants