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

Adopt numpy docstrings #3840

Closed
tkknight opened this issue Sep 11, 2020 · 14 comments
Closed

Adopt numpy docstrings #3840

tkknight opened this issue Sep 11, 2020 · 14 comments
Assignees
Milestone

Comments

@tkknight
Copy link
Contributor

📚 Documentation

We should adopt the NumPy docstrings for Iris over the current Google Style docstrings.

There is a sphinx extension that allows both to be supported when creating the API documentation, named Napolean. This would allow the Iris codebase to gradually move to numpy docstrings and avoid a big bang approach.

We could also:

  • Improve the docstrings as we port
  • Update the developers guide on the documentation to state the intent

Can the @SciTools/iris-devs can agree this? (btw, I'm not proposing this goes into Iris 3.0.0)

@bouweandela
Copy link
Member

To make it easier to have an opinion on this, could you shortly describe why the numpy style is better than the google style?

@pp-mo
Copy link
Member

pp-mo commented Sep 14, 2020

BTW the current style, as described in https://scitools-iris.readthedocs.io/en/latest/developers_guide/documenting/docstrings.html , is not in fact simply "google style". I think we designed it to our own best needs.
The biggest difference, I think, is our using separate "Args" and "Kwargs" sections (instead of labelling individual args as "optional" in the description). This is of course Python-specific. In future, I personally would like to start using keyword-only parameters for some things, as I think this really enhances code transparency in some cases. I don't know how we will be representing that in docstrings, perhaps it is just obvious from the signature (but so is the arg/kwarg distinction).

FWIW I personally prefer the more google-like style, simply because it uses less vertical space, which makes it more readable in the code files.

@bjlittle bjlittle self-assigned this Oct 1, 2020
@bjlittle bjlittle added Peloton 🚴‍♂️ Target a breakaway issue to be caught and closed by the peloton and removed New: Documentation labels Oct 1, 2020
@tkknight
Copy link
Contributor Author

tkknight commented Oct 1, 2020

See #3871. This PR enables the napolean extensions to allow both Google and Numpy docstrings. It does not force adoption of numpy - but it could with a change to the config in conf.py.

@bjlittle
Copy link
Member

bjlittle commented Oct 9, 2020

Personally, I think the iris doc-strings, in general, are a bit of mess.

Unless there is a hint of rigour or standardisation, which there isn't at the moment, we've ended up with quite a wide ranging combination of "google style" (and I use that in the loosest possible terms) doc-strings.

After the documentation, users will hit the doc-strings to get details on functionality, and it's really is a bit of a lottery regarding the quality and format that they get.

In general, I think it's high time that we had some doc-string hygiene, so it makes sense to me that if we're revisiting the content and quality of doc-strings, that we also address the format of the doc-strings.

I think that we could argue either way for google style vs numpy style, but that fact that I can follow a numpy docstring guide, off the bat, wins hands down for me - there's a standard there to follow, and we don't have to reinvent the wheel, and it plugs into napolean just fine.

So I'm 👍 for numpy style, which personally, I find a lot clearer and it renders just fine in readthedocs. So I'd happily iterate on slowing migrating iris doc strings across - I can see the end user benefit for the effort, particularly when it comes to hinting at the dtype of arguments

@pp-mo
Copy link
Member

pp-mo commented Oct 9, 2020

Agree with all the above, except I marginally prefer google style for compactness, as I already said !
If this pushes us to conform to a single standard, and stick to it, I'm 👍 👍 👍 for that !

@bjlittle
Copy link
Member

bjlittle commented Oct 9, 2020

@SciTools/developers Are you guys happy for me to add this issue to the backlog for 3.1 ?

@rcomer
Copy link
Member

rcomer commented Oct 9, 2020

I have no objection to the proposal but I'm glad I'm not the one who would have to implement it!

I must admit I've never gone looking for guidelines on how the docstrings should look. I just look at what else is in the module and try to make my new bit consistent. That approach would obviously be problematic if we are part way through porting...

@pp-mo
Copy link
Member

pp-mo commented Oct 9, 2020

add this issue to the backlog for 3.1 ?

I may have overdone the enthusiasm slightly.
But I don't think this would be too hard to do, really. It just needs prioritising.

@rcomer
Copy link
Member

rcomer commented Oct 9, 2020

  • Improve the docstrings as we port

That could potentially incorporate #3292, which is currently in the 3.1 milestone/project.

@tkknight
Copy link
Contributor Author

tkknight commented Oct 9, 2020

If we agree to use numpy docstrings, we can review the default napolean settings: https://github.com/SciTools/iris/blob/master/docs/iris/src/conf.py#L135-L147 to ensure it fits our needs.

We could also look into automating checks for non numpy docstrings so we can check for compliance over time.

@bjlittle bjlittle removed the Peloton 🚴‍♂️ Target a breakaway issue to be caught and closed by the peloton label Oct 9, 2020
@bjlittle bjlittle added this to the v3.1.0 milestone Oct 9, 2020
@bjlittle
Copy link
Member

bjlittle commented Oct 9, 2020

Although this all seems like a lot of work, if we just divvy it up between a whole bunch of core devs, and people iterate on individual modules so that we don't tread on each others toes, we could easily swarm on this and nail it pretty quickly.

That would be lovely 😄

@pp-mo
Copy link
Member

pp-mo commented Oct 9, 2020

automating checks for non numpy docstrings

I rather assumed it already did that.
Will it accept anything without complaint then, or will it detect and warn of (some) non-compliant docstrings ?

@tkknight
Copy link
Contributor Author

hi @pp-mo,

Napolean was introduced to also allow numpy docstrings, as some we being pulled in from outside of the Iris docs. This was enabled via https://github.com/SciTools/iris/blob/master/docs/iris/src/conf.py#L136. See the full PR #3871.

A quick tests shows that if I set napoleon_google_docstring = False (currently is True) it appears to make no difference. It will still parse all existing google docstrings, I was hoping it would flag up all google docstrings so we can easily check for conformance.

However, if any are badly formatted docstrings it will flag as a warning, and as all sphinx build warnings are now configured to be promoted to errors, it will fail the build (this is to ensure no warnings creep in).

@bjlittle bjlittle modified the milestones: v3.1.0, v3.2.0 Aug 4, 2021
@tkknight
Copy link
Contributor Author

Let's close this issue and track progress of adoption via #4721

Repository owner moved this from To Do to Done in Iris v3.2.0 Apr 28, 2022
@pp-mo pp-mo mentioned this issue May 4, 2023
91 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

No branches or pull requests

6 participants