-
-
Notifications
You must be signed in to change notification settings - Fork 36
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
Comments
👍 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. |
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? |
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? |
Could we have something like a stale bot that auto closes applications that have had no activity after a month? |
This comment from @dstansby here
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? |
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. |
Mostly playing devils advocate here but looking over the review criteria:
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. |
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. |
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.
The text was updated successfully, but these errors were encountered: