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

A few edits to the reviewer template #31

Merged
merged 4 commits into from
May 21, 2020
Merged

A few edits to the reviewer template #31

merged 4 commits into from
May 21, 2020

Conversation

lwasser
Copy link
Member

@lwasser lwasser commented Jun 20, 2019

ok let's try this again

@leouieda @luizirber @marskar @lheagy @mbjoseph @jlpalomino @choldgraf please have a look at this PR and let me know what you think! edits welcome!!

@lwasser lwasser changed the title a few edits to the reviewer template [WIP] a few edits to the reviewer template Jun 20, 2019

The README should include, from top to bottom:

- [ ] The package name
- [ ] Badges for continuous integration and test coverage, the badge for pyOpenSci peer-review once it has started (see below), a repostatus.org badge, and any other badges. If the README has many more badges, you might want to consider using a table for badges, see this example, that one and that one. Such a table should be more wide than high.
- [ ] Short description of goals of package, with descriptive links to all vignettes (rendered, i.e. readable, cf the documentation website section) unless the package is small and there’s only one vignette repeating the README.
- [ ] Short description of goals of package
- [ ] Dscriptive links to all vignettes (rendered, i.e. readable, cf the documentation website section) unless the package is small and there’s only one vignette repeating the README.
Copy link

Choose a reason for hiding this comment

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

Typo: Dscriptive -> Descriptive


- [ ] The documentation is easy to find and understand
- [ ] The need for the package is clear
- [ ] All functions have documentation and associated examples for use
Copy link

@marskar marskar Jun 20, 2019

Choose a reason for hiding this comment

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

How do you feel about suggesting/requiring the use of the sphinx coverage extension?
sphinx.ext.coverage is to doc coverage as coverage.py is to test coverage.

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 usually -1 to required specific tools. I'm definitely +1 to recommend them.
I'm of an opinion that, asking too much from a new scientist/developer may drive the person away. IMO a single script deployed with flit on PyPI is a good candidate for PyOpenSci.

Copy link
Member Author

Choose a reason for hiding this comment

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

i agree that requiring specific extensions isn't ideal. ropensci also agrees. For coverage they just ask for reasonable test coverage and then an explanation associated with coverage numbers that are low. They do not specify requirements for docs coverage. we are using autodoc and doctest to cover doc tests and probably wouldn't want to switch given that works for us!!

Copy link

Choose a reason for hiding this comment

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

+1 on not requiring specific extensions.

I would also suggest breaking this item into two

  • Example use
  • API documentation

As they might live in different parts of the docs, and I think we want to encourage reviewers to explicitly look for both. Also, it for examples, it might make more sense to have a single example that uses multiple functions rather than one example per function (and the current language suggests we want one-per function).

One last point, I quite like the JOSS Good, OK, Bad (not acceptable) breakdown for items like the API docs, installation instructions, and testing: https://joss.readthedocs.io/en/latest/review_criteria.html#api-documentation. It might be worth thinking about hosting a similar page for pyOpenSci to give reviewers and editors a few decision criteria when questions of "where is the bar" come up.

@choldgraf
Copy link
Contributor

Is it ready for a review now? (usually when I see WIP, I assume it's not yet ready for reviewers)

@lwasser lwasser changed the title [WIP] a few edits to the reviewer template A few edits to the reviewer template Jun 24, 2019
@lwasser
Copy link
Member Author

lwasser commented Jun 24, 2019

@choldgraf this is ready for review. this set of edits attempts to address some of the issues in the erdappy submission! pyOpenSci/software-submission#1 feel free to edit / suggest changes as you see fit.

Copy link

@lheagy lheagy left a comment

Choose a reason for hiding this comment

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

This is looking solid! I think the additional "Usability" heading is quite helpful. 🎉

@@ -47,20 +47,30 @@ The package includes all the following forms of documentation:
Readme requirements
The package meets the readme requirements below:

- [ ] Package has a README.md file in the root directory.
- [ ] Package has a user-friendly README.md file in the root directory.
Copy link

Choose a reason for hiding this comment

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

I hesitate to add qualitative descriptors without defining a bit of what we mean by that. As an editor, it is tricky to help facilitate discussions where there is a disagreement about the "user-friendliness" of a doc, as it can start coming down to opinions. In my opinion, the description below of what the README should include is quite matter-of-fact, so it is easier to make decisions on. Just my 2 cents :)

Copy link
Member

@ocefpaf ocefpaf Jun 25, 2019

Choose a reason for hiding this comment

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

I agree.

Also, IMO, READMEs are a dev/power user resource and not user documentation substitute. Users need fully developed docs while a README is the quick "this is how to install and start hacking on it" kind of thing. BTW, most users do not even find the README b/c they do not interact with the package via the source or the repo. (Not to mentioned that most packages do not ship the README with the source!)

Note that requiring complex doc-like README we may force people to have redundant info on docs and README that may go stable very quickly and actually create more harm than good. It is not hard to find updated docs and stale READMEs b/c of that.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok! so remove the user-friendly language? i am fine with that and hear you @ocefpaf about the potential for redundancy as well!!

@@ -47,20 +47,30 @@ The package includes all the following forms of documentation:
Readme requirements
The package meets the readme requirements below:

- [ ] Package has a README.md file in the root directory.
- [ ] Package has a user-friendly README.md file in the root directory.

The README should include, from top to bottom:
Copy link

Choose a reason for hiding this comment

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

how strict do we want to be on the "top to bottom"? For example, if the readme has a badge for the docs, do we also require a heading that points to the docs? Personally, I would be inclined to leave this simply at "The README should include" and give the authors some discretion to decide the exact order.

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 OK with:

  • Badges for test coverage and repostatus.org badge.

We can recommend those but requiring them does not really add quality to the scientific package. While adding them does add a maintenance burden to the package author.

  • descriptive links to all vignettes (rendered, i.e. readable, cf the documentation website section)

That creates some redundancy with the docs and I'm not a fan. It get stale very quickly and increases the maintenance burden.

  • If applicable, how the package compares to other similar packages and/or how it relates to other packages

Nice to have but should not be mandatory.

  • Citation information

Nice to have but should not be mandatory.

Copy link
Member Author

Choose a reason for hiding this comment

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

i hear you both @lheagy @ocefpaf !!
Ok so it sounds like

  1. We should discuss badges! id definitely want a pyopensci one there which won't be any effort to maintain. but maybe we talk as a group about what other things we want to be visible. From my perspective, visibly showing some commitment to code coverage via testing and CI is important in a package. i could see that being presented in different ways. Thoughts on this?
  2. A clean link to the docs via the readme. with an option of links to vignettes? i think @ocefpaf we are trying to figure out a way to say " it should be easy to find documentation and various parts of it specifically (like vignettes) that a user would need to figure out how to use a package". we are trying to leave space for a reviewer to provide usability input. if docs are hard to navigate we need to help submitters improve that usability component.
  3. agreed... we ask for that relationship to other packages when the package is submitted. i agree it doesn't need to be mandatory. let's plan to chat more about this in our next meeting!!
  4. i think the citation information is very important. Especially with all of the issues now with scientific software not being "counted" towards scientists cv...in academic contexts. i would like to require that IF others agree.

Copy link
Member

Choose a reason for hiding this comment

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

definitely want a pyopensci one there which won't be any effort to maintain.

👍

From my perspective, visibly showing some commitment to code coverage via testing and CI is important in a package. i could see that being presented in different ways. Thoughts on this?

I see value in the coverage metric only for big projects with many contributors. Small projects with less people this value is lost b/c it is very easy to get 100% coverage and not tests what is important. That is why I prefer a policy of "recommend but do not require" when it comes to automated coverage. With that said, I do believe we should review packages paying attention to the coverage! We just should not outsource that to an automated tests/badge.

A clean link to the docs via the readme. with an option of links to vignettes? i think @ocefpaf we are trying to figure out a way to say " it should be easy to find documentation and various parts of it specifically (like vignettes) that a user would need to figure out how to use a package". we are trying to leave space for a reviewer to provide usability input. if docs are hard to navigate we need to help submitters improve that usability component.

👍 to adding links. I guess I'm not sure what 'descriptive links' mean? Maybe "clear links to the documentation" is clearer? (No pun indented.)

i think the citation information is very important. Especially with all of the issues now with scientific software not being "counted" towards scientists cv...in academic contexts. i would like to require that IF others agree.

While I agree that it is very important it is a step that some research software engineers will not comply due to problems that may arise with the scientists in the team, institution policies, etc. We may close the door to important scientific software b/c some people may be stuck in old academia practices that will prevent them to generate a "citation" for the package. I can elaborate more (this is based on personal experience) at SciPy next week.


- [ ] The documentation is easy to find and understand
- [ ] The need for the package is clear
- [ ] All functions have documentation and associated examples for use
Copy link

Choose a reason for hiding this comment

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

+1 on not requiring specific extensions.

I would also suggest breaking this item into two

  • Example use
  • API documentation

As they might live in different parts of the docs, and I think we want to encourage reviewers to explicitly look for both. Also, it for examples, it might make more sense to have a single example that uses multiple functions rather than one example per function (and the current language suggests we want one-per function).

One last point, I quite like the JOSS Good, OK, Bad (not acceptable) breakdown for items like the API docs, installation instructions, and testing: https://joss.readthedocs.io/en/latest/review_criteria.html#api-documentation. It might be worth thinking about hosting a similar page for pyOpenSci to give reviewers and editors a few decision criteria when questions of "where is the bar" come up.

@lwasser lwasser merged commit d860d01 into master May 21, 2020
@NickleDave NickleDave deleted the reviewer-template branch September 23, 2022 16:09
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

Successfully merging this pull request may close these issues.

5 participants