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

FIX parsing of type-only return params #175

Merged
merged 1 commit into from Apr 10, 2019

Conversation

timhoffm
Copy link
Contributor

@timhoffm timhoffm commented May 2, 2018

Fixes #72.

The sections "Parameters" and "Returns" have different semantics for single-element parameter headers. For "Parameters", the header is the name, for "Returns" the header is the type. This was not reflected in the code as both sections used the same parser.

Note: This is just a quick write-up how the code should work. I've not yet let run the tests (they may need fixing as well as mentioned in #72). But for brevity I'll let CI decide on that.

@timhoffm
Copy link
Contributor Author

timhoffm commented May 2, 2018

Note: CI currently fails because of pip changes (#174).

@jnothman
Copy link
Member

jnothman commented May 3, 2018 via email

@timhoffm
Copy link
Contributor Author

timhoffm commented May 3, 2018

As said above, I didn't run any tests so far myself. It's apparent that one parser for "Parameters" and "Returns" cannot get both right. I'm confident that the proposed code change itself is correct. What has to be still shown is that the calling code and tests work with that (it might be that they partly compensate for the original bug or that this effect is not tested as all). I thought I'd use the tests in CI for that, but CI is already prematurely failing for different reasons.

@timhoffm
Copy link
Contributor Author

timhoffm commented May 3, 2018

Rebased onto master.

Note: The tests do currently fail. Waiting for #176 before doing any further changes to fix tests.

@timhoffm timhoffm force-pushed the fix-type-only-return-param branch from c1c0126 to 4d858d0 Compare May 5, 2018 14:36
@timhoffm
Copy link
Contributor Author

timhoffm commented May 5, 2018

PR updated.

Single element returns params such as:

Returns
-------
int
    The return value.

were detected as names. I.e. int was considered a name. This logical error has been fixed such that int is now a type and the name is empty.

As a consequence, int is not formatted bold anymore. This is consistent with the formatting of types in patterns like x : int and a prerequisite for type references like :class:`MyClass` to work in this position.

@mattip
Copy link
Member

mattip commented Jun 13, 2018

ping?

@fredrik-1 fredrik-1 mentioned this pull request Jun 28, 2018
@larsoner
Copy link
Collaborator

larsoner commented Apr 2, 2019

@timhoffm can you rebase? Then I can take a look and hopefully merge

Copy link
Collaborator

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Other than the conflict, LGTM +1 for merge

@rgommers
Copy link
Member

I've taken the liberty of fixing the merge conflict. The only nontrivial change was deciding where the new heading Receives goes; I added it to 'Returns', 'Yields', 'Raises', 'Warns'.

@rgommers rgommers added this to the v0.9.0 milestone Apr 10, 2019
Copy link
Collaborator

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

LGTM +1 for merge

@rgommers rgommers merged commit af6690a into numpy:master Apr 10, 2019
@rgommers
Copy link
Member

Okay in it goes. Thanks @timhoff, and sorry it took a while! And thanks @larsoner for reviewing

@timhoff
Copy link

timhoff commented Apr 10, 2019 via email

@rgommers
Copy link
Member

hmm odd, I thought I tab-completed that. Sorry for the noise @timhoff. And thanks @timhoffm !

@timhoffm timhoffm deleted the fix-type-only-return-param branch April 11, 2019 05:45
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.

Anonymous return values have their types populated in the name slot of the tuple.
6 participants