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

Agree on and document in the affiliated package review section how we handle responses to reviewers #337

Open
Cadair opened this issue Sep 12, 2022 · 8 comments

Comments

@Cadair
Copy link
Member

Cadair commented Sep 12, 2022

This has come up in #302 and #335 that the reviewer makes a suggestion, it generates discussion and/or fixes and then there's no clear path forward. We should fix that.

@dstansby
Copy link
Member

👍 to giving the applicant one chance to respond to the review. I think this should be time limited (two weeks?) so the scope is limited to making quick and minor fixes.

@Cadair
Copy link
Member Author

Cadair commented Sep 12, 2022

Hmm, I am not sure if having someone come back month(s) later and having to open a new issue or just resurrecting the old one is better?

@nabobalis
Copy link
Contributor

I feel like resurrecting the old one is better unless they want to open a new one, highlight the old issue and ask for a full review again?

@wtbarnes
Copy link
Member

Could we have something like a stale bot that auto closes applications that have had no activity after a month?

@Cadair
Copy link
Member Author

Cadair commented Sep 29, 2022

This comment from @dstansby here

I forgot to put the verson reviewed in my original review, but I reviewed v0.3.0. It seems like there hasn't been an updated release with any changes, but if a new release is made I would be happy to re-review.

raises and interesting question, which is do we require a new release before a re-review? We put the version reviewed on the review, so I can certainly see the logic of having that as an "epoch", but on the other hand many things on the review list don't actually require a release (for example adding the CoC to the readme in the sunkit-instruments review).

Thoughts?

@dstansby
Copy link
Member

I think we definitely should require a re-release, since there's no point fixing and improving stuff if it's not in a released version users can easily get at. Reviewing a release isn't just reviewing the package on PyPi, it's reviewing the git repository at that tag, so having the (e.g.) CoC present in the tag that's reviewed is important.

@Cadair
Copy link
Member Author

Cadair commented Sep 29, 2022

Mostly playing devils advocate here but looking over the review criteria:

  • Functionality - release needed
  • Integration - release needed
  • Documentation - mixed, but probably should have a release
  • Testing - not release dependent
  • Duplication - release
  • Community - not release dependent
  • Development Status - regular releases are part of this criteria so discount.

A lot of things could use a new release, but not everything, especially if the changes are in a category where a release isn't really relevant (i.e. testing changes) requiring them to do a release which basically contains nothing seems like a lot of effort for minimal gain? Obviously, in some cases it would definitely be needed.

@dstansby
Copy link
Member

I disagree, and think everything that requires a change in the version control should require a new release. This is mainly because a package is not just the code - it's the code together with the docs and testing providing context. To a user code with no tests is different to code with comprehensive tests, even if functionally it does the same thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants