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

Initial PR template #613

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Initial PR template #613

wants to merge 3 commits into from

Conversation

mfisher87
Copy link
Member

@mfisher87 mfisher87 commented Jun 25, 2024

Addresses #281 but doesn't resolve it. We still need issue templates. We should break that ticket in to pieces.

cc @Sherwin-14


📚 Documentation preview 📚: https://earthaccess--613.org.readthedocs.build/en/613/

<details><summary>Pull Request (PR) draft checklist - click to expand</summary>

- [ ] Populate a descriptive title. Ensure the title does not look like "Updated
README.md".
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we can help the contributor find a more useful way to describe their PR, in addition to saving what to avoid? Maybe something like:

  • Populate a descriptive title, including what is particular to this PR. For example, instead of "Updated README.md", use a title such as "Added testing details to the contributor section of the README".

- A clear description of the change you are proposing.
- Links to any issues resolved by this PR with text in the PR description, for
example "closes #1".
- [ ] Please review our
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps this should be the first item in the checklist?

Copy link
Collaborator

Choose a reason for hiding this comment

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

And the next one, "Ensure an issue exists representing the problem being solved in this PR.", could be the second item?

Copy link
Collaborator

@itcarroll itcarroll left a comment

Choose a reason for hiding this comment

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

I'm not sure whether it should be in the template or contributing, but can we say what to expext in terms of maintainer review? A contributor should not expect any comments or action until either help is requested or the PR is marked as ready for review.

Comment on lines +36 to +38
- [ ] Click the "Ready for review" button at the bottom of the "Conversation" tab in GitHub
once these requirements are fulfilled. Don't worry if you see any test failures in
GitHub at this point!
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd make this item regular text, not a checkbox. It's an instruction more than a "todo" and will be indicated by the PR status.

Comment on lines +72 to +73
homepage](https://github.com/nsidc/earthaccess), make your change on the fork, then open
a pull request from your fork and follow the instructions populated in the text box.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
homepage](https://github.com/nsidc/earthaccess), make your change on the fork, then open
a pull request from your fork and follow the instructions populated in the text box.
homepage](https://github.com/nsidc/earthaccess), create a branch with your changes in the fork, then open
a draft pull request from your fork. Starting a pull request provides additional instructions and requirements, and
there is no harm in starting a draft pull request while still developing.

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.

None yet

3 participants