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

Remove our fork of the numpydoc instructions in favor of the official page #9727

Merged
merged 2 commits into from
Dec 9, 2019

Conversation

astrofrog
Copy link
Member

While working on #9726 I noticed that there were a number of out-of-date sections in the docs, one of which was the sentence that mentioned that we ship a fork of numpydoc in astropy. In fact we use vanilla numpydoc and don't bundle it anymore. At the time we set up our docs, the numpydoc format specification was not available on a nicely presented page so we forked them - however, the specification can now be found at https://numpydoc.readthedocs.io/en/latest/format.html which is nice and readable.

Our version has some small differences, mainly a couple of examples where we chose astropy-related variable/type names, but other than that our version is just an out of date version of the official one. So this PR is to propose simply removing our fork in favor of linking to the official one. This will also make it much simpler to stay up to date as the numpydoc package and format grow some new features.

I'm not sure if we need a changelog entry? If people are on board with this, would it make sense to include in 4.0?

@astrofrog astrofrog added the Docs label Dec 6, 2019
@pllim
Copy link
Member

pllim commented Dec 6, 2019

I vote for adding a change log, in case someone links to that page and is confused the link is broken now.

Also, are you sure our rules are exactly the same as Numpy's? Did we resolve the "array-like" vs "array_like"?

@pllim pllim added this to the v4.0.1 milestone Dec 6, 2019
@pllim
Copy link
Member

pllim commented Dec 6, 2019

Failure is real: development_workflow.rst:315:undefined label: doc-rules

@astrofrog
Copy link
Member Author

@pllim - I think the only actual real difference in rule was array_like but that got changed back in #9684 and merged. But in any case we can take this PR as being a check for whether people are happy to fully align with the numpydoc rules now (which would make sense since we already decided to use vanilla numpydoc rather than our fork)

I'll fix the doc warning, and I can also add back an orphan page giving the link to the new page in case anyone has linked to the old doc rules from other websites.

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

FWIW, I am okay with this. I have no preference which way you want to fix the broken link problem.

@astrofrog
Copy link
Member Author

Note to maintainers: let's wait for a few other approvals before considering whether to merge this

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Very much in favour!

@@ -1,569 +1,8 @@
.. doctest-skip-all
.. _doc-rules:
:orphan:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a big fan of keeping orphan pages. Why not delete it all?

Copy link
Member Author

Choose a reason for hiding this comment

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

In case any other projects were linking to it, to avoid breaking links for now.

Copy link
Member

@Cadair Cadair Dec 8, 2019

Choose a reason for hiding this comment

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

I am pretty sure sunpy is linking to this. Nope, maybe not.

Copy link
Member

Choose a reason for hiding this comment

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

Then they better update their links? 🤷‍♀

Copy link
Member

Choose a reason for hiding this comment

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

I have merged this now, but my comment still stands. I don't think we should keep orphaned pages around, downstream should keep track of their links and update as needed.

CHANGES.rst Outdated
@@ -335,7 +335,9 @@ astropy.wcs
Other Changes and Additions
---------------------------


- The page in the documentatin describing the astropy docstring rules has now
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need any changelog for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

typo: "documentation" (if the changelog entry stays there :))

@bsipocz
Copy link
Member

bsipocz commented Dec 7, 2019

Release wise I would be happier with this in 4.0 than in 4.0.1.

@astrofrog astrofrog modified the milestones: v4.0.1, v4.0 Dec 8, 2019
@astrofrog
Copy link
Member Author

I removed the changelog entry and re-milestoned to 4.0

@bsipocz
Copy link
Member

bsipocz commented Dec 9, 2019

There is overall happiness with 5 approvals, so I go ahead now and merge this.

Thank you @astrofrog

@bsipocz bsipocz merged commit 6e5c1aa into astropy:master Dec 9, 2019
bsipocz added a commit that referenced this pull request Dec 11, 2019
Remove our fork of the numpydoc instructions in favor of the official page
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.

6 participants