-
Notifications
You must be signed in to change notification settings - Fork 59
Review 2 (reviews open): Add: documentation section to package guide #19
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
Review 2 (reviews open): Add: documentation section to package guide #19
Conversation
Update documentation/contributing-file.md Co-authored-by: Alexandre Batisse <[email protected]> Update documentation/index.md Co-authored-by: Alexandre Batisse <[email protected]> Update documentation/index.md Co-authored-by: Alexandre Batisse <[email protected]> Update documentation/package-documentation-sphinx.md Co-authored-by: Alexandre Batisse <[email protected]> Update documentation/readme-file-best-practices.md Co-authored-by: Alexandre Batisse <[email protected]> Update documentation/readme-file-best-practices.md Co-authored-by: Alexandre Batisse <[email protected]> Update documentation/readme-file-best-practices.md Co-authored-by: Alexandre Batisse <[email protected]> Update documentation/readme-file-best-practices.md Co-authored-by: Alexandre Batisse <[email protected]> Update documentation/readme-file-best-practices.md Co-authored-by: Alexandre Batisse <[email protected]>
Update documentation/index.md Co-authored-by: Ariane Sasso <[email protected]> Update documentation/package-documentation-sphinx.md Co-authored-by: Ariane Sasso <[email protected]>
Co-authored-by: David Nicholson <[email protected]> Update documentation/package-documentation-sphinx.md Co-authored-by: David Nicholson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great write up! I have a few suggestions if I may 😃
|
||
Your README file should have the following information: | ||
- [ ] The name of the package | ||
- [ ] Badges for the packages current published version, documentation and test suite build. (OPTIONAL: test coverage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal take, I don't like badges about CI because it does not represent what users would get. Most users would get a stable release, hence this has little value. I even think that it can send a wrong message to some people if they see a red badge for some time. Depending on the CI jobs, that does not necessarily mean the project is broken all the time.
@@ -0,0 +1,251 @@ | |||
# Python Package Documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: could be worth revisiting this and adding nbgallery as a front end with nbsphinx - will provide a much nicer front end for peope to use https://nbsphinx.readthedocs.io/en/0.8.11/subdir/gallery.html thanks to @astrojuanlu for the tip!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: this is a really important item but i'm going to add this as an issue #35 so we can circle back and work on it given the size if this pr currently is massive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge step forward for our docs docs 🙂
This review was spread out over three days, I promise I did try to remove any of the extra grumpiness from late-night reviewing. Hope that it's helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge step forward for our docs docs 🙂
This review was spread out over three days, I promise I did try to remove any of the extra grumpiness from late-night reviewing. Hope that it's helpful
@all-contributors please add @tupui for doc, design |
I've put up a pull request to add @tupui! 🎉 |
Thanks 😊 |
Co-authored-by: Pamphile Roy <[email protected]>
Co-authored-by: Pamphile Roy <[email protected]>
One thought on TOC/document structure: might be nice to group some things together and break up some of the pages for skimming/reference?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like :) thanks for all your work writing this. Hopefully these are useful, sorry if they are all very persnickety, thus is the nature of feedback.
My big global comment is that some macro-level reorganization would be nice: as is the order feels a little random and I am not left with a clear picture of what is required by pyOpenSci re: documentation since it's buried around on a few pages. I see that several files were removed and combined here, and so not sure what the deal is there but I suggested a simple 2-layer nested structure in one of my comments that I feel like would be helpful to keep the docs expandable and also be able to navigate around for someone interested in submitting.
thanks again :) all my comments 'optional' to take or leave as they're useful or not.
## Documentation landing page best practices | ||
|
||
To make it easy for users to find what they need quickly, all packages should | ||
have 4 elements on the home page of their documentation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get a little confused here at the order - above docs are described as having two (2.5 with tutorials) parts, then here it seems like now we are saying docs have 4 parts and then restates the two parts. maybe unify this into a single statement of docs structure and then say "landing page should link out to all top-level things in the basic structure."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i totally understand why. I suspect the new organization will be more clear. I tried to clarify the sections... but let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh and also this is just a suggest for design of package landing page. i added an image below it. (From geopandas)
Return | ||
------ | ||
extent_json: A GeoJSON style dictionary of corner coordinates | ||
for the extent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
creates an additional bullet point in the return list (see image below this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sneakers-the-rat i don't understand this comment and what image you are referring to. what are you referring to ? do you mean in the return when you run help()
?
|
||
``` | ||
|
||
```{figure} ../images/sphinx-rendering-extent-to-json-earthpy.png |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might as well just write a dummy function and call autodoc on it (and then show the autodoc directive as well?) and show the generated docs from the docstring? might be easier to maintain than having to eg. update the screenshot if something changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i could this is from earthpy i could pull from docs and such - we did run doctest ... let me look at this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i think i see what you're saying. i no longer maintain earthpy. we could add this to our examplePy package repo... not in this PR but it's a good idea for the future. then we have full control and can use it for doc updates. i'll make a note of this
* A CODE_OF_CONDUCT.md file | ||
* A development guide (if possible) | ||
|
||
## What a CONTRIBUTING.md file should contain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing with each of these headings
Contributing
License
Code of Conduct
* A link to licensing information found in your README file. | ||
* A link to a development guide if you have one | ||
|
||
## What the development guide for your Python package should contain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear how this is different than the Contributing.md and makes me feel like i'm supposed to write another guide. I would make this a subheading underneath contributing and shorten the header to Developer Docs
or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh it is another guide! the community seems to be moving away from contributing and development being conflated. So contributing guides are more about understanding what types of contributions are welcome - issues, pull requests, questions, and how to submit them. Whereas a dev guide describes the dev workflow - fewer contributors will install pre-commit hooks and want to know about build tools. but that should be documented.
this is particularly important if you want to onboard a new contributor OR for pyOpenSci if a maintainer steps down and we need to find someone new - we want the workflow documented.
this page tho is also split out as we discussed so i hope the entire section is clearer. pyopensci does NOT require a dev guide but i think it will be important when we need to find new maintainers for packages.
software license that is approved | ||
by the Open Software Initiative (OSI). OSI's website has a | ||
[list of popular licenses](https://opensource.org/licenses). GitHub also has a | ||
[handy tool](https://choosealicense.com/) for choosing a license. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linking to the tool is great. Maybe broad strokes description of the basic "big cuts" differences in licenses and perhaps a recommendation or a statement about which licenses are most common/expected by pyOpenSci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sneakers-the-rat are you open to writing a bit more about licenses and COC directly via a pr? we could either add it here OR we could add it once this is merged. When i push my latest revisions up this PR is going to get messy but id' welcome your making direct content additions to it or in a new pr when this is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure! of course. lmk when it's in a more settled state to add to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome! i'm about to push a really big set of fixes. and then however you want to contribute you are welcome to. You could either submit a PR on top of this pr? Or you can add inline changes here ! however you'd like to do this i'm open . I now have a coc page and a license page. (I may be missing sufficient information around citation.)
|
||
If you choose your license through GitHub, you can also automatically get a copy of the license file to add to your repository. | ||
|
||
## The CODE_OF_CONDUCT.md file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a lil more on what the code of conduct should do/what is typically in them would also be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wasn't sure what to add on this page. Right now i have contributing and COC as one page because they are both short sections. Maybe we can pull this out as an issue as well and add more to it once this PR is merged? i appreciate this feedback .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created an issue here #37 that we can then circle back on - great suggestion!
Co-authored-by: David Nicholson <[email protected]>
Co-authored-by: David Nicholson <[email protected]>
Co-authored-by: David Nicholson <[email protected]>
Co-authored-by: David Nicholson <[email protected]>
@all-contributors please add @sneakers-the-rat for code, design |
I've put up a pull request to add @sneakers-the-rat! 🎉 |
@sneakers-the-rat i really appreciate your thoughts on this and the suggested reorg. it's much cleaner now. i think i was stuck working in an old structure but also trying to move a lot of content across into a new format. i needed your fresh eyes to reconsider organization. THANK YOU for the huge contribution here. i am SO SO SO appreciative. |
ok y'all. i've gotten through MANY of the comments. |
I think the last BIG picture comment that i don't feel confident on is how we make it clear what pyos REQUIRES vs just guidance and best practices following @sneakers-the-rat comments. as such i'll open an issue about that as well. |
ok yall. i am going to MERGE this PR. we've i think had well over 400 comments and contributions here from various folks and i appreciate it all. I will merge but please know this is a LIVING document. it is not the final - this is just round 1. ok round 2 for this content. We will merge and then update / fix enhance as we go. Thank you all for being a part of this vision and for supporting this effort! ✨ |
This pr builds on top of #14 which will be closed. It contains a lot of new and re-organized information based upon feedback.
I would like feedback by January 16 (which is a holiday in the US). I will hope to MERGE this pr that week by Jan 18th.