Skip to content

Correctly validate parameters under the "Other Parameters" section #337

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

Merged
merged 5 commits into from
Nov 5, 2021
Merged

Conversation

dcbr
Copy link
Contributor

@dcbr dcbr commented Oct 25, 2021

The "PR01" error is falsely raised when validating doc strings with parameter definitions under the "Other Parameters" section (e.g. for infrequently used parameters). This PR makes sure such parameters are correctly recognized by the validation script (and an extra test is added that previously failed but is now properly supported).

@dcbr dcbr changed the title Fix invalid "PR01" error when parameter is specified in "Other Parame… Correctly validate parameters under the "Other Parameters" section Oct 25, 2021
@dcbr
Copy link
Contributor Author

dcbr commented Oct 29, 2021

So doc.example.foo is failing because in that example non-existent parameters are specified in the "Other Parameters" section. The only_seldom_used_keyword can be added to the parameter list I guess, as the name clearly suggests the infrequent usage. However I'm unsure what the meaning of the common_parameters_listed_above parameter was. Does this mean it is/should be allowed to repeat parameters from the "Parameters" section in the "Other Parameters" section?

@larsoner
Copy link
Collaborator

larsoner commented Nov 1, 2021

Does this mean it is/should be allowed to repeat parameters from the "Parameters" section in the "Other Parameters" section?

I'm not sure what it means either but I don't think it should be repeated. Your fix of adding only_seldom_used_keyword sounds good to me

@dcbr
Copy link
Contributor Author

dcbr commented Nov 2, 2021

All right, I had to make the "PR03" test a bit more complicated, as otherwise that example still failed. The new test makes sure the order of the parameters within each section is consistent with the order in the signature (whereas previously it was using a concatenation of the parameters in each section).

@larsoner
Copy link
Collaborator

larsoner commented Nov 2, 2021

(whereas previously it was using a concatenation of the parameters in each section).

I wonder if this part was intentional, in that in theory the infrequently used parameters should come at the end of the args/kwargs list in the signature...

@dcbr
Copy link
Contributor Author

dcbr commented Nov 2, 2021

Actually before this PR they were not considered at all (completely neglected), then my initial changes concatenated them. However this leads to an error for the following signature (taken from the foo example):
def foo(var1, var2, *args, long_var_name='hi', only_seldom_used_keyword=0, **kwargs):
Where all parameters except the only_seldom_used_keyword one are described in the "Parameters" section first and then the only_seldom_used_keyword one is described in the "Other Parameters" section. This then leads to the PR03 error as the **kwargs are mentioned before the only_seldom_used_keyword parameter.

An alternative fix would have been to keep the concatenation and move the **kwargs to the "Other Parameters" section. But I'm not sure this is desirable either, as it would effectively force to describe any remaining variable keyword parameters in the "Other Parameters" section whenever any previous keyword parameter is described there. Anyway I have only been using numpydoc fairly recently, so I will follow your advice :)

@dcbr dcbr mentioned this pull request Nov 4, 2021
8 tasks
@dcbr
Copy link
Contributor Author

dcbr commented Nov 5, 2021

Reverted some of the previous changes, so now once again the signature order has to exactly match the concatenation of the Parameters order and the Other Parameters order.
@larsoner this is ready for review then.

@larsoner larsoner merged commit 1668dd9 into numpy:main Nov 5, 2021
@larsoner
Copy link
Collaborator

larsoner commented Nov 5, 2021

Thanks @dcbr

@jarrodmillman jarrodmillman added this to the 1.2.0 milestone Jan 9, 2022
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