-
Notifications
You must be signed in to change notification settings - Fork 34
enh(author-guide): Add discussion about when a package is good enough for review #361
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
Conversation
Co-authored-by: Jonny Saunders <sneakers-the-rat@protonmail.com>
|
|
||
| - **Functions and classes (the API) are stable or close to reaching stability** after the review is complete. "Stable" means that the scope of the package is settled and well defined, and that backwards-incompatible changes to the API will be managed carefully. | ||
| - **Major changes and refactoring can still be carried out after review** but should not be a frequent occurrence. | ||
| - **Most functionality should be clearly documented and tested**. At the submission stage, all major functions should be stable enough to be fully documented and tested. The README should make a strong case for the package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We reference the README specifically in the next bullet so I'm not sure it needs to be included here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I made some edits below - @eigenbrot see what you think. I removed the README statement as it was redundant!
| Your package is ready for review when: | ||
|
|
||
| - **Functions and classes (the API) are stable or close to reaching stability** after the review is complete. "Stable" means that the scope of the package is settled and well defined, and that backwards-incompatible changes to the API will be managed carefully. | ||
| - **Major changes and refactoring can still be carried out after review** but should not be a frequent occurrence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording of this bullet is kind of opposite of the other bullets; they all describe a desired state of affairs while this bullet bolds a thing that should be minimized. Perhaps something like "The scope and factoring of the code are relatively stable. Major changes and refactor can still be carried out but should not be frequent." could work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree - and also it relates to the bullet above where it says
Functions and classes (the API) are stable or close to reaching stability**. "Stable" means that the scope of the package is settled and well defined. Future, backwards-incompatible changes to the API will be managed carefully.
This is really getting to the package's development being mature enough to be reviewed. I wonder if we want to just remove this bullet. We won't be checking on the package after review for API stability. But we do want it to be stable coming into to us. What do you think about just removing this bullet @eigenbrot @eliotwrobson ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good call. Especially after your rework of the previous bullet I think that it alone gets the point across. I'm fine removing this bullet.
how-to/author-guide.md
Outdated
| ## 4. Submit your package for peer review | ||
| ## 4. Prepare your package for peer review | ||
|
|
||
| Please consider the best time in your package's development to submit for review. Your package should be sufficiently mature so that reviewers can review all essential aspects. Keep in mind that review may result in major changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first two sentences here read a bit awkward to me. Maybe something like "Packages submitted for review should be mature enough that reviewers can review all of its essential aspects." could read better?? I'm not sure the current first sentence is doing much lifting.
Also, it might look good to bold "may result in major changes."
|
|
||
| - **Functions and classes (the API) are stable or close to reaching stability** after the review is complete. "Stable" means that the scope of the package is settled and well defined, and that backwards-incompatible changes to the API will be managed carefully. | ||
| - **Major changes and refactoring can still be carried out after review** but should not be a frequent occurrence. | ||
| - **Most functionality should be clearly documented and tested**. At the submission stage, all major functions should be stable enough to be fully documented and tested. The README should make a strong case for the package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - **Most functionality should be clearly documented and tested**. At the submission stage, all major functions should be stable enough to be fully documented and tested. The README should make a strong case for the package. | |
| - Before your submit, be sure that **most functionality in your package is stable and clearly documented and tested**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this looks great! We could consider removing "Before you submit" because that's kind of implied by the heading of the list. Either way works for me.
lwasser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - i've suggested some inline changes that @eigenbrot was getting at in the comments. @eliotwrobson i think you can implement these and merge if you are comfortable doing so! if not ping me on slack to have another look and we can get this merged that way!!
eigenbrot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the edits, @lwasser! I'm happy to apply them myself, but I'm not sure if that's more a job for @eliotwrobson as the PR author. If there's a standard way we do things please let me know.
| Your package is ready for review when: | ||
|
|
||
| - **Functions and classes (the API) are stable or close to reaching stability** after the review is complete. "Stable" means that the scope of the package is settled and well defined, and that backwards-incompatible changes to the API will be managed carefully. | ||
| - **Major changes and refactoring can still be carried out after review** but should not be a frequent occurrence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good call. Especially after your rework of the previous bullet I think that it alone gets the point across. I'm fine removing this bullet.
|
|
||
| - **Functions and classes (the API) are stable or close to reaching stability** after the review is complete. "Stable" means that the scope of the package is settled and well defined, and that backwards-incompatible changes to the API will be managed carefully. | ||
| - **Major changes and refactoring can still be carried out after review** but should not be a frequent occurrence. | ||
| - **Most functionality should be clearly documented and tested**. At the submission stage, all major functions should be stable enough to be fully documented and tested. The README should make a strong case for the package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this looks great! We could consider removing "Before you submit" because that's kind of implied by the heading of the list. Either way works for me.
|
@eigenbrot I would greatly appreciate you jumping in and pushing to this PR! We're a little shorthanded right now so anything to spread out the workload is a huge plus 🙏 |
Rewording, etc. Co-authored-by: Leah Wasser <leah@pyopensci.org>
|
Looks like all the changes are done. Will go ahead and merge and if there are any more changes we want happy to merge a follow up. Huge thanks @eigenbrot !! |
Closes #101
Used AI agent for summarizing discussions and updates to guide.