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

FIX: Update and cleanup the peer review section of our guide #148

Merged
merged 21 commits into from
Dec 28, 2022

Conversation

lwasser
Copy link
Member

@lwasser lwasser commented Nov 29, 2022

This PR updates the review guides for authors, editors, editor in chief and reviewers. It also cleans up content creating include files for all templated items that may end up in a few places. Finally it ensures that no one page is too long and breaks out content such as finding reviewers into separate pages. this will just make the content easier to read and discover.

@lwasser
Copy link
Member Author

lwasser commented Nov 29, 2022

This is a really big PR but it's not new content i'm just cleaning up content. it is ready for review. @NickleDave pinging you here and @arianesasso too

@NickleDave
Copy link
Contributor

NickleDave commented Nov 30, 2022

👍 edit: just saw you commented in Slack

will prioritize #146 and then review this before Dec 25th at the latest

@lwasser
Copy link
Member Author

lwasser commented Nov 30, 2022

awesome and this is a lot of content between the two pr's so if you need more time please take it. thank you @NickleDave !!

I am going to ignore any comments left until I see a message that you have completed your review. :)
i will move on to the package guide for the time being and will forget about these pr's until it's time to work on them again!
THANK YOU

@@ -0,0 +1,22 @@
```markdown
Copy link
Member Author

Choose a reason for hiding this comment

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

@leouieda @ocefpaf @xmnlab @yuvipanda @lheagy @xmnlab one. more item to review. This is short. it's the submission template where we collect information about the package before review. if you have time to look at it and provide any feedback please do.

checks.md Outdated Show resolved Hide resolved
appendices/editor-in-chief-checks.md Outdated Show resolved Hide resolved
appendices/package-approval-template.md Outdated Show resolved Hide resolved
appendices/review-template.md Outdated Show resolved Hide resolved
appendices/review-template.md Outdated Show resolved Hide resolved
appendices/review-template.md Outdated Show resolved Hide resolved
appendices/reviewer-request-template.md Outdated Show resolved Hide resolved
software-peer-review-guide/editor-in-chief-guide.md Outdated Show resolved Hide resolved
software-peer-review-guide/reviewer-guide.md Outdated Show resolved Hide resolved
software-peer-review-guide/editors-guide.md Outdated Show resolved Hide resolved
index.md Show resolved Hide resolved
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.

Guide is looking better and better! These are mostly little suggested edits, hope some are helpful

appendices/editor-in-chief-checks.md Outdated Show resolved Hide resolved
appendices/review-template.md Outdated Show resolved Hide resolved
appendices/review-template.md Outdated Show resolved Hide resolved
appendices/review-template.md Outdated Show resolved Hide resolved
checks.md Outdated Show resolved Hide resolved
software-peer-review-guide/editors-guide.md Show resolved Hide resolved
software-peer-review-guide/intro.md Outdated Show resolved Hide resolved
software-peer-review-guide/intro.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Batalex Batalex left a comment

Choose a reason for hiding this comment

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

Thank you for your work 🙏

about-peer-review/how-peer-review-works.md Outdated Show resolved Hide resolved
about-peer-review/intro.md Outdated Show resolved Hide resolved
about-peer-review/intro.md Outdated Show resolved Hide resolved
about-peer-review/policies-guidelines.md Outdated Show resolved Hide resolved
you with a citable, [Crossref digital object identifier (DOI)](https://www.crossref.org/).
pyOpenSci aligns closely with the broad mission of
JOSS to provide maintainers with credit for their open source work. However,
our mission is also more focused. pyOpenSci not open source maintainers getting academic credit for their work. We also support:
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean

pyOpenSci [is not just?] open source [...]

?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes!

appendices/scope.md Outdated Show resolved Hide resolved
appendices/scope.md Outdated Show resolved Hide resolved
consider submitting a pre-submission inquiry first and explain why the package is a
fork rather than an independent parent repository.

### Example 2: Vendored dependencies
Copy link
Contributor

@Batalex Batalex Dec 22, 2022

Choose a reason for hiding this comment

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

I have a question, where do we stand about git submodules?

A submodule is not a proper dependency, buuuut it allows to upgrade its content

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 a really good question. So this topic came up with a previous review - pyOpenSci/software-submission#60 we had to close the issue and reject because they had forked the repo that they were vendoring and then were writing code on top of that fork. The issue there was multi-faceted but frm our perspective it would be VERY hard to find a new maintainer for that kind of custom workflow. I think if it had been a git submodule it could have been ok.

Its funny this came up as it's really an edge case but i'm learning that i think python will be full of edge cases that R doesn't see as often!

technically I don't know how a submodule works for packaging. Can you tell me @Batalex when you implement a submodule and package to tool are you then shipping the tool with both the vendored submodule AND the package itself vs installing as a dependency? many thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I have only tried once, so I am no expert.
A git submodule allows you to add secondary git folders inside your project. The commit histories would remain separate, however.

Since API wrappers and bindings for other tools are in scope for pyOpenSci, we could end up in a situation where a python package would ship some native lib in a submodule. Any time you want to release a new version, you can git pull the submodule to get the latest commit.

This usage would be pretty marginal though, especially considering that one may use conda to install non-python dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you so much, @Batalex !! your thoughts here are super helpful!

software-peer-review-guide/editors-guide.md Outdated Show resolved Hide resolved
software-peer-review-guide/editors-guide.md Outdated Show resolved Hide resolved

- [ ] **A statement of need** clearly stating problems the software is designed to solve and its target audience in README
- [ ] **Installation instructions:** for the development version of package and any non-standard dependencies in README
- [ ] **Vignette(s)** demonstrating major functionality that runs successfully locally
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there was a discussion about vignette(s) in slack started by @NickleDave (and now I get it haha 😅). I personally never heard this term before. Maybe we could give an option: Vignette(s)/Snippets (or something more general?).

Copy link
Member Author

Choose a reason for hiding this comment

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

@arianesasso yes that word is specific to the R community. In fact if you package an R package there is a vignette function. i'm so used to using it from years of working with R but i wonder if we just want to use short tutorials for Python as most are familiar with it to avoid cognitive overload? i'm somewhat torn on it but most people i meet who don't use python don't know that word?

Copy link
Contributor

Choose a reason for hiding this comment

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

Heey, thanks for clarifying 😄 . I would personally prefer something more general, as you said: short tutorials because then the R community and others could also understand. You could mention (if necessary) something like: short tutorials (for R users, something like vignettes).

- [ ] **Installation instructions:** for the development version of package and any non-standard dependencies in README
- [ ] **Vignette(s)** demonstrating major functionality that runs successfully locally
- [ ] **Function Documentation:** for all user-facing functions
- [ ] **Examples** for all user-facing functions
Copy link
Contributor

Choose a reason for hiding this comment

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

I am also unsure what differentiates Vignette(s) from Examples 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

yea... @arianesasso your questions are just making me think we should use "short tutorials" what do you think about that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree :).

appendices/review-template.md Outdated Show resolved Hide resolved
appendices/review-template.md Outdated Show resolved Hide resolved
appendices/review-template.md Outdated Show resolved Hide resolved
software-peer-review-guide/reviewer-guide.md Outdated Show resolved Hide resolved
software-peer-review-guide/reviewer-guide.md Outdated Show resolved Hide resolved
software-peer-review-guide/reviewer-guide.md Outdated Show resolved Hide resolved
software-peer-review-guide/reviewer-guide.md Outdated Show resolved Hide resolved
about-peer-review/pyopensci-related-joss-ropensci.md Outdated Show resolved Hide resolved
1. We support long term maintenance of packages. If the maintainer needs to step down, we will ensure a new maintainer takes over OR sunset and remove the package from our ecosystem.
1. We provide a welcoming forum for you to ask questions and get help with maintaining your package as needed.

JOSS reviews are also [more limited in scope](https://joss.readthedocs.io/en/latest/review_criteria.html)
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
JOSS reviews are also [more limited in scope](https://joss.readthedocs.io/en/latest/review_criteria.html)
JOSS reviews are also [more limited in scope](https://joss.readthedocs.io/en/latest/submitting.html?highlight=scope#submission-requirements)

Copy link
Member Author

Choose a reason for hiding this comment

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

Github is doing weird things today but i did fix this locally.

about-peer-review/pyopensci-related-joss-ropensci.md Outdated Show resolved Hide resolved
- **Data deposition:** Tools for depositing data in scientific research repositories.
- **Reproducibility:** Tools to scientists ensure that their research is reproducible. E.g. version control, automated testing, or citation tools.
- **Geospatial:** Packages focused on the retrieval, manipulation, and analysis of spatial data.
- **Education:** Packages to aid with instruction.
Copy link
Member Author

Choose a reason for hiding this comment

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

@arianesasso education is also here because of me :) we could totally revisit this list maybe in a separate PR? discuss it first in an issue if you'd like to open an issue about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I will do that! I also saw that in a different thread, so I guess it is time 😄 .

Copy link
Contributor

Choose a reason for hiding this comment

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

plus one!

This also came up in pyOpenSci/python-package-guide#21 -- maybe tag that issue too if you open a new one?

I think the person opening it didn't realize it was on python-packaging-guide instead of peer-review-guide

They specifically talk about the way we list categories now possibly limiting what people might consider in scope
pyOpenSci/python-package-guide#21 (comment)

In addition to revisiting categories, maybe it's worth thinking about what language we use to introduce this section.
Maybe we want to say something like "below is a list of some categories that fall within scope of pyOpenSci and our overall mission of enabling open science with Python. This list is not meant to be exclusive!"

... but I'll save that discussion for a new issue, don't mean to add noise to this review

lwasser and others added 5 commits December 27, 2022 10:55
Co-authored-by: Alexandre Batisse <[email protected]>

Co-authored-by: David Nicholson <[email protected]>
Co-authored-by: David Nicholson <[email protected]>
Co-authored-by: Alexandre Batisse <[email protected]>
@lwasser lwasser force-pushed the peer-review-edits branch 2 times, most recently from 9a15751 to 0e3a9fb Compare December 27, 2022 18:04
lwasser and others added 2 commits December 27, 2022 11:08
Update software-peer-review-guide/editors-guide.md

Co-authored-by: David Nicholson <[email protected]>

Update software-peer-review-guide/editors-guide.md

Co-authored-by: David Nicholson <[email protected]>

Apply suggestions from code review

Co-authored-by: David Nicholson <[email protected]>

Update about-peer-review/how-peer-review-works.md

Co-authored-by: Alexandre Batisse <[email protected]>

Update about-peer-review/code-of-conduct.md

Co-authored-by: Ariane Sasso <[email protected]>

Update about-peer-review/how-peer-review-works.md

Co-authored-by: Ariane Sasso <[email protected]>

Update about-peer-review/how-peer-review-works.md

Co-authored-by: Ariane Sasso <[email protected]>

Update about-peer-review/how-peer-review-works.md

Co-authored-by: Ariane Sasso <[email protected]>

Update about-peer-review/intro.md

Co-authored-by: Ariane Sasso <[email protected]>

Apply suggestions from code review

Co-authored-by: Alexandre Batisse <[email protected]>
Co-authored-by: Ariane Sasso <[email protected]>

Update about-peer-review/pyopensci-related-joss-ropensci.md

Update about-peer-review/pyopensci-related-joss-ropensci.md

Co-authored-by: Alexandre Batisse <[email protected]>

Update about-peer-review/pyopensci-related-joss-ropensci.md

Co-authored-by: Ariane Sasso <[email protected]>

Update about-peer-review/pyopensci-related-joss-ropensci.md

Co-authored-by: Ariane Sasso <[email protected]>

Update about-peer-review/pyopensci-related-joss-ropensci.md

Co-authored-by: Ariane Sasso <[email protected]>

Update about-peer-review/pyopensci-related-joss-ropensci.md

Update about-peer-review/pyopensci-related-joss-ropensci.md

Co-authored-by: Alexandre Batisse <[email protected]>

Update about-peer-review/pyopensci-related-joss-ropensci.md

Co-authored-by: Ariane Sasso <[email protected]>

Edits to peer review guide on massive reorganization of guide

Edits to peer review guide

Co-authored-by: Ariane Sasso <[email protected]>

Update appendices/editor-in-chief-checks.md

Co-authored-by: Ariane Sasso <[email protected]>

Update appendices/pre-review-package-requirements.md

Co-authored-by: Ariane Sasso <[email protected]>

Update appendices/pre-review-package-requirements.md

Co-authored-by: Ariane Sasso <[email protected]>

Update appendices/reviewer-request-template.md

Co-authored-by: Alexandre Batisse <[email protected]>

Update appendices/reviewer-request-template.md

Co-authored-by: Ariane Sasso <[email protected]>

Update appendices/reviewer-request-template.md

Co-authored-by: Alexandre Batisse <[email protected]>

Apply suggestions from code review

Co-authored-by: Ariane Sasso <[email protected]>

Update appendices/package-approval-template.md

Co-authored-by: Ariane Sasso <[email protected]>

Apply suggestions from code review

Co-authored-by: Ariane Sasso <[email protected]>
Co-authored-by: Alexandre Batisse <[email protected]>

Apply suggestions from code review

Co-authored-by: Ariane Sasso <[email protected]>

Update software-peer-review-guide/editor-in-chief-guide.md

Co-authored-by: Ariane Sasso <[email protected]>

Update software-peer-review-guide/editor-in-chief-guide.md

Co-authored-by: Ariane Sasso <[email protected]>

Update software-peer-review-guide/editor-in-chief-guide.md

Co-authored-by: Ariane Sasso <[email protected]>

Update software-peer-review-guide/editor-in-chief-guide.md

Co-authored-by: Ariane Sasso <[email protected]>

Apply suggestions from code review

Co-authored-by: Ariane Sasso <[email protected]>

Update software-peer-review-guide/intro.md

Co-authored-by: Ariane Sasso <[email protected]>

Update software-peer-review-guide/reviewer-guide.md

Co-authored-by: Ariane Sasso <[email protected]>

Apply suggestions from code review

Co-authored-by: Ariane Sasso <[email protected]>
@lwasser
Copy link
Member Author

lwasser commented Dec 28, 2022

ok y'all. this MASSIVE MASSIVE pr that has been worked on for a solid few months is now being merged. ALL - once this is merged, I welcome you to read the document online, submit issues and or pr's for small fixes to the text at ANY TIME. this is not the final final. It's a final draft in my eyes but this guide will be living which means we will continue to update. i think this just sets us up for a usable working guidebook. and ideally maybe we won't have MASSIVE restructuring after this - rather finessing of text and guidelines as we need to.

Many thanks for all of the help with this heavy heavy lift !!

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.

5 participants