Skip to content

Allow param lists to split on ': ' rather than ' : ' #78

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

Open
jnothman opened this issue Nov 21, 2016 · 15 comments
Open

Allow param lists to split on ': ' rather than ' : ' #78

jnothman opened this issue Nov 21, 2016 · 15 comments

Comments

@jnothman
Copy link
Member

It is very easy to mistakenly separate parameter names from their type description by ': ' instead of the correct ' : '. There are at least 260 instances in Scikit-learn, for instance, and Joblib appears to use : without a preceding space as a standard. This variation is unsurprising when See Also, for instance, does not require a space before its colons.

Would we expect great ramifications were we to change it to allow a single colon followed by whitespace as delimiter?

@jnothman
Copy link
Member Author

Ping @scikit-learn

@stefanv
Copy link
Contributor

stefanv commented Nov 21, 2016

How about we split correctly but issue a warning?

@jnothman
Copy link
Member Author

See, by the way, my attempt (via grep, sed, git apply, git add --patch) to fix such errors in scikit-learn at scikit-learn/scikit-learn#7920

I considered a warning, and don't mind it. I'd actually rather something that output a git patch :) Main problem is that I don't think NumpyDocstring has enough context information to make the warning very informative.

@stefanv
Copy link
Contributor

stefanv commented Nov 22, 2016

That grep script is truly horrific ;)

@jnothman
Copy link
Member Author

jnothman commented Nov 22, 2016

I did try implementing it in numpydoc, but got sick at the thought of putting all the context info in.

This worked :)

@stefanv
Copy link
Contributor

stefanv commented Nov 22, 2016

So, if you have something that works, would fixing numpydoc still be useful to you, or should we just add that "fixer" into the README?

@jnothman
Copy link
Member Author

I think fixing numpydoc is worthwhile. I wanted a way to resolve these
quickly so that we can then use numpydoc to check for inconsistencies
between signature and docstrings.

On 23 November 2016 at 09:52, Stefan van der Walt notifications@github.com
wrote:

So, if you have something that works, would fixing numpydoc still be
useful to you, or should we just add that "fixer" into the README?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEz62wrbGFfo-Wr74W5XcbYkNK4TWy7ks5rA3IdgaJpZM4K4qEh
.

@stefanv
Copy link
Contributor

stefanv commented Nov 23, 2016

I think this and other discussions around similar requests has helped settle the issue in my mind: we should stick to one docstring format, and rather help those who want to fix their projects with a linter-type tool.

@jnothman
Copy link
Member Author

jnothman commented Nov 23, 2016

The problem with that statement is that the docstring format is somewhat inconsistent: see also allows leading space or none; parameters allows with leading space only.

@jnothman
Copy link
Member Author

But yes, I'm happy with a linter/fixer.

@stefanv
Copy link
Contributor

stefanv commented Nov 23, 2016

The docstring format seems consistent? https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt

@jnothman
Copy link
Member Author

But the implementation is not similarly restrictive:
https://github.com/numpy/numpydoc/blob/master/numpydoc/docscrape.py#L255

On 23 November 2016 at 12:42, Stefan van der Walt notifications@github.com
wrote:

The docstring format seems consistent? https://github.com/numpy/
numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#78 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEz605jA4bkSoRCM1srNv1e9fEiMCeQks5rA5oOgaJpZM4K4qEh
.

@jnothman
Copy link
Member Author

I'll submit a PR for a linter/fixer.

@stefanv
Copy link
Contributor

stefanv commented Nov 23, 2016

Thanks @jnothman, I'd be happy to review that.

@anntzer
Copy link
Contributor

anntzer commented Oct 3, 2017

I think the original proposal should be rejected for the same reason as mentioned in #87 (comment) (+ following discussion).

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

No branches or pull requests

3 participants