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

Updated Pull Request Checklist #592

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Brijeshthummar02
Copy link

Description

here’s an improved version of your .md checklist file with added sections for documentation standards, details on expected documentation, and coverage reports.
Fixes #591

Checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@MicahGale MicahGale added the documentation Improvements or additions to documentation label Nov 14, 2024
@MicahGale MicahGale self-requested a review November 14, 2024 13:37
@MicahGale MicahGale self-assigned this Nov 14, 2024
Copy link
Collaborator

@MicahGale MicahGale 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 @Brijeshthummar02 for tackling this.

For some context, I am the main contributor and usually opening most of the PRs myself, and I feel like I have been getting a little bit lazy with this. This issue was mostly a quick note I mostly wrote to myself to update the checklist so I get less lazy with my PRs. So sorry that there was some ambiguity in the issue.

If you would like to make more contributions we have a tag for issues that might be easier to start with: good first issue. Out of curiosity: how did you come across MontePy? It is a bit of a niche library.

- [ ] I have made corresponding changes to the documentation (if applicable)
- [ ] I have added tests that prove my fix is effective or that my feature works (if applicable)
- [ ] I have performed a self-review of my own code.
- [ ] The code follows the standards outlined in the [development documentation](https://idaholab.github.io/MontePy/developing.html).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this alone adds much value, as the guidelines are already linked.

I would say remove this line and then go through the checklist here, and make sure everything is coverered: https://www.montepy.org/developing.html#merge-checklist

- [ ] I have added tests that prove my fix is effective or that my feature works (if applicable)
- [ ] I have performed a self-review of my own code.
- [ ] The code follows the standards outlined in the [development documentation](https://idaholab.github.io/MontePy/developing.html).
- [ ] I have made corresponding changes to the documentation, providing clear details on the added or modified functionality (if applicable).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking this would have some sub bullet points:

    - [ ] I have documented all added Classes and methods
    - [ ] For infrastructure updates I have updated the developer's guide
    - [ ] For significant new features I have added a section to the getting started guide. 

- [ ] The code follows the standards outlined in the [development documentation](https://idaholab.github.io/MontePy/developing.html).
- [ ] I have made corresponding changes to the documentation, providing clear details on the added or modified functionality (if applicable).
- [ ] I have added tests that prove my fix is effective or that my feature works (if applicable).
- [ ] I have checked that my code achieves the required test coverage, and I have included coverage reports (if applicable).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm realizing this is hacky solution right now. I am working to debug why coveralls isn't sending status updates.
I just want the merger (usually me) to add a link to the coveralls report. A link to coveralls: https://coveralls.io/github/idaholab/MontePy/ should be included to be able to quickly get there.

@MicahGale
Copy link
Collaborator

@Brijeshthummar02 I wanted to check on this. Are you able to make these changes? If I don't hear back in a week I will close this PR.

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

Successfully merging this pull request may close these issues.

Update pull request checklist
2 participants