Skip to content

REVIEW 1 (reviews closed) - ADD: Documentation section to our packaging guide #14

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 11 commits into from
Closed

REVIEW 1 (reviews closed) - ADD: Documentation section to our packaging guide #14

wants to merge 11 commits into from

Conversation

lwasser
Copy link
Member

@lwasser lwasser commented Nov 30, 2022 •
edited
Loading

This PR is ready for review! @NickleDave and everyone. would look feedback by December 23 if that is reasonable.

@lwasser lwasser force-pushed the update-documentation branch from 172bcd8 to 8370452 Compare November 30, 2022 18:25
@lwasser lwasser mentioned this pull request Nov 30, 2022
Closed
@lwasser lwasser requested a review from NickleDave November 30, 2022 21:18
@lwasser lwasser added new-content New feature or request 🚀 ready-for-review labels Nov 30, 2022
SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
documentation/contributing-file.md Outdated

* Any guidelines that you have in place for users submitting issues, pull requests or asking questions.
* A link to your code of conduct
* A link to a development guide if you have one
Copy link
Contributor

Choose a reason for hiding this comment

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

What else:

  • Information about licensing, especially if someone pulls a code into a package,
  • A link to social networks or forms where contributions can be made.

Copy link
Member Author

Choose a reason for hiding this comment

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

gosh yes - i thought i had added this website but obviously not! https://www.contributor-covenant.org/

but we do ask for the license information in the readme file rather than the contributor file. Maybe look at that section and see what you think?.

SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
documentation/contributing-file.md Outdated
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe here it would be nice to point out that license is needed if package is published on PyPI and conda :) So no license == no publication!

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. i think we have a few complex use cases to consider with licensing. In some of our open forums in the past some have mentioned that adding specific licenses can be hard in some environments... agency or government came up specifically. So while i think we want to strongly encourage it, it is also important to consider those use cases. We are not a publisher at the end of the day but we do want software to get cited.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is what we wrote related to this

To be reviewed by pyOpenSci your project should use an open source software license that is approved by the Open Software Initiative (OSI).

i do think however if push came to shove and adding a formal license was not possible we would have a discussion. i might ask ropensci how they deal with that.

SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
documentation/readme-file-best-practices.md
Ideally your GitHub repository's name is also the name of your package. The more
self explanatory that name is, the better.

### ✔️ Badges for current package version, continuous integration and test coverage
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a badge with a license too (?)

Copy link
Member Author

Choose a reason for hiding this comment

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

huh good point. i haven't seen license badges used too widely given there is a file in the repo called LICENSE. id say if someone really wants that it could be optional. I think the tests, CI docs building etc and python versions are probably more important.

SimonMolinsky
SimonMolinsky reviewed Dec 2, 2022
View reviewed changes
SimonMolinsky
SimonMolinsky suggested changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@SimonMolinsky SimonMolinsky left a comment

Choose a reason for hiding this comment

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

I've put some comments to change/fix small typos in docs.

lwasser
lwasser commented Dec 13, 2022
View reviewed changes
lwasser
lwasser commented Dec 14, 2022
View reviewed changes
documentation/contributing-file.md Outdated
@@ -0,0 +1,92 @@
# Contributing, License and Code of Conduct Files in your Python Open Source Package
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: this file in an open PR that i will close once i'm sure all of the content is moved here needs to be considered when updating this file. there is some good stuff about the contributors covenent website and such there

Copy link
Member Author

Choose a reason for hiding this comment

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

ALSO TODO: add a note about github issue templates in this section...

Batalex
Batalex reviewed Dec 15, 2022
View reviewed changes
NickleDave
NickleDave reviewed Dec 20, 2022
View reviewed changes
NickleDave
NickleDave reviewed Dec 21, 2022
View reviewed changes
Copy link
Contributor

@NickleDave NickleDave left a comment

Choose a reason for hiding this comment

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

Looks great, our documentation guide is well documented 😛
Feels like we are almost ready to start reviewing a lot of packages!!! 😀
Hope this feedback helps some

documentation/package-documentation-sphinx.md Outdated
A few Sphinx themes that are commonly used in the Scientific Python community that you might
consider include:
While you are free to use whatever sphinx theme you prefer,
the most common modern templates that recommend for the Python scientific
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the most common modern templates that recommend for the Python scientific
some templates with a modern look-and-feel that are commonly used by the Python scientific

I'm more verbose I know, trying to avoid "common modern" as a noun stack and to sound a little less prescriptive

@NickleDave
Copy link
Contributor

NickleDave commented Dec 21, 2022 •
edited
Loading

@lwasser adding suggestion from Slack discussion to explain how to write vignettes, and outline tools: sphinx-gallery, nbsphinx + myst-nb

I have an example of using jupytext + myst-nb here
https://raw.githubusercontent.com/vocalpy/crowsetta/main/doc/howto/howto-user-format.md
Basically a matter of adding yaml front matter so myst-nb finds the file.
This is nice because you can run and test in Jupyter lab and have the myST directives in your vignette, for when you will want to ref other parts of the docs.
A slight gotcha is that you need to use myst-nb in your extensions and not myst_parser, as I'm doing here:
https://github.com/vocalpy/crowsetta/blob/28fd13613c3d08d0592ca522b86b87e669efd3b8/doc/conf.py#L42

@lwasser
Copy link
Member Author

lwasser commented Dec 22, 2022

This Pr is missing a section about accessibility related to documentation. just noting that and once it's merged i can create a new pr on top of it to address accessibility.

@lwasser
Copy link
Member Author

lwasser commented Dec 22, 2022

@NickleDave does that example approach actually run the code tutorial code and render it? if it doesn't i'd suggest we just suggest sphinx gallery.

@lwasser
Copy link
Member Author

lwasser commented Dec 22, 2022

@NickleDave i love adding this content. i was thinking... so this pr doesn't get huge like the peer review guide did:) what do you think about focusing just on what's here. And what i can do is open an issue to add both tutorials and accessibility which is totally missing from this pr. that way we can get this merged and work on that separately.

@lwasser lwasser mentioned this pull request Dec 22, 2022
Closed
2 tasks
@lwasser lwasser force-pushed the update-documentation branch 3 times, most recently from 0ad92eb to 8ff8003 Compare December 28, 2022 23:29
lwasser and others added 9 commits December 28, 2022 16:31
@lwasser
Fix: Review edits from @lwasser and @arianesasso
f0e1a3c
@lwasser
Fix: add docstring examples
d409543
@lwasser @Batalex
Fix: review edits to documentation section w @Batalex
0431311 
Update documentation/contributing-file.md

Co-authored-by: Alexandre Batisse <alexandre.batisse@hey.com>

Update documentation/index.md

Co-authored-by: Alexandre Batisse <alexandre.batisse@hey.com>

Update documentation/index.md

Co-authored-by: Alexandre Batisse <alexandre.batisse@hey.com>

Update documentation/package-documentation-sphinx.md

Co-authored-by: Alexandre Batisse <alexandre.batisse@hey.com>

Update documentation/readme-file-best-practices.md

Co-authored-by: Alexandre Batisse <alexandre.batisse@hey.com>

Update documentation/readme-file-best-practices.md

Co-authored-by: Alexandre Batisse <alexandre.batisse@hey.com>

Update documentation/readme-file-best-practices.md

Co-authored-by: Alexandre Batisse <alexandre.batisse@hey.com>

Update documentation/readme-file-best-practices.md

Co-authored-by: Alexandre Batisse <alexandre.batisse@hey.com>

Update documentation/readme-file-best-practices.md

Co-authored-by: Alexandre Batisse <alexandre.batisse@hey.com>
@lwasser @arianesasso
Fix: review edits to documentation from @arianesasso
ec2d032 
Update documentation/index.md

Co-authored-by: Ariane Sasso <ariane.sasso@gmail.com>

Update documentation/package-documentation-sphinx.md

Co-authored-by: Ariane Sasso <ariane.sasso@gmail.com>
@lwasser @NickleDave
Fix: Review edits to doc section from @NickleDave
8ced667 
Co-authored-by: David Nicholson <NickleDave@users.noreply.github.com>

Update documentation/package-documentation-sphinx.md

Co-authored-by: David Nicholson <NickleDave@users.noreply.github.com>
@lwasser
Fix: edits to doc section from @SimonMolinsky
8cc1bc3
@lwasser
rebase
dfea3b3
@lwasser
Fix: numerous cleanup edits to contributing and readme files
38fad88
@lwasser
Fix: breakout docs into separate pages with separate goals and update…
70410f5 
… left bar links
@lwasser lwasser force-pushed the update-documentation branch from 8ff8003 to 70410f5 Compare December 28, 2022 23:32
@lwasser lwasser marked this pull request as draft December 29, 2022 18:21
@lwasser lwasser changed the title ADD: Documentation section to our packaging guide REVIEW 1 (reviews closed) - ADD: Documentation section to our packaging guide Dec 29, 2022
@lwasser lwasser mentioned this pull request Dec 29, 2022
Merged
@lwasser
Copy link
Member Author

lwasser commented Jan 6, 2023

@all-contributors
please add @arianesasso for documentation, design.
please add @SimonMolinsky for documentation, design

@allcontributors
Copy link
Contributor

@lwasser

This project's configuration file has malformed JSON: .all-contributorsrc. Error:: Unexpected token } in JSON at position 383

@lwasser
Copy link
Member Author

lwasser commented Jan 6, 2023 •
edited
Loading

@all-contributors
please add @arianesasso for documentation, design.
please add @SimonMolinsky for documentation, design.

@lwasser
Copy link
Member Author

lwasser commented Jan 6, 2023

@all-contributors
please add @arianesasso for documentation, design.

@allcontributors allcontributors bot mentioned this pull request Jan 6, 2023
Merged
@allcontributors
Copy link
Contributor

@lwasser

I've put up a pull request to add @arianesasso! 🎉

@lwasser
Copy link
Member Author

lwasser commented Jan 6, 2023

@all-contributors
please add @Batalex for documentation, design.
please add @SimonMolinsky for documentation, design.

@allcontributors allcontributors bot mentioned this pull request Jan 6, 2023
Merged
@lwasser
Copy link
Member Author

lwasser commented Jan 6, 2023

@all-contributors
please add @SimonMolinsky for documentation, design.

@allcontributors allcontributors bot mentioned this pull request Jan 6, 2023
Merged
@allcontributors
Copy link
Contributor

@lwasser

@SimonMolinsky already contributed before to doc, design

@lwasser
Copy link
Member Author

lwasser commented Jan 19, 2023

closing this PR as all comments from #19 have been addressed and it's merged!

@lwasser lwasser closed this Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arianesasso arianesasso arianesasso left review comments

@Batalex Batalex Batalex left review comments

@NickleDave NickleDave NickleDave left review comments

@SimonMolinsky SimonMolinsky SimonMolinsky requested changes

Assignees
No one assigned
Labels
new-content New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lwasser @NickleDave @arianesasso @Batalex @SimonMolinsky