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

Add draft github pull-request template #53

Merged
merged 14 commits into from
Jul 11, 2024

Conversation

leifdenby
Copy link
Member

@leifdenby leifdenby commented Jun 3, 2024

The aim of this PR is to add a pull-request template for use on github when add pull-requests to the common repo. The aim of my initial commit is to start the discussion on how we would like pull-requests to progress when collaborating on neural-lam.

As you will see from the template I have created so far the aim is to:

  • create placeholders for the PR creator to add context so we know what the PR is for
  • detail the steps of getting the PR reviewed and merged into the repo

Looking forward to hearing your thoughts all :)

Here's a direct link so you can see it rendered: https://github.com/leifdenby/neural-lam/blob/maint/add-github-pr-template/.github/pull_request_template.md

This commit adds a draft pull-request template for use on github when
add pull-requests to the common repo. The aim of this commit is to start
the discussion on how we would like pull-requests to progress when
collaborating on neural-lam.
@leifdenby leifdenby self-assigned this Jun 3, 2024
Copy link
Collaborator

@sadamov sadamov left a comment

Choose a reason for hiding this comment

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

Thanks, this will prove very useful! I added some smaller discussion points.

Comment on lines 25 to 28
- [ ] the code readable
- [ ] the code well tested
- [ ] the code documented
- [ ] the code easy to maintain
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
- [ ] the code readable
- [ ] the code well tested
- [ ] the code documented
- [ ] the code easy to maintain
- [ ] the code is readable
- [ ] the code is well tested
- [ ] the code is documented
- [ ] the code is easy to maintain

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it clear how to test "easy to maintain"? Maybe we should be a bit more specific here (e.g. modular, using existing open-source libraries, ...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think most of these can be expanded on and made more concrete. But that then turns into more of a coding style guide, that probably fits better somewhere else (related to my other comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

we can also leave "easy to maintain" out? The other three cover this some degree and this isn't explicit enough about how we measure this that the comment is probably superfluous

Copy link
Member Author

Choose a reason for hiding this comment

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

I will remove "easy to maintain" since that is hard to measure objectively, we can always adapt this template later if we find it is missing something or doesn't quite work :)

@SimonKamuk
Copy link
Contributor

Great idea with the template, and good suggestions by @sadamov. I have just added a small comment above, and have no further changes - great work!

Comment on lines 25 to 28
- [ ] the code readable
- [ ] the code well tested
- [ ] the code documented
- [ ] the code easy to maintain
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think most of these can be expanded on and made more concrete. But that then turns into more of a coding style guide, that probably fits better somewhere else (related to my other comment).

leifdenby and others added 3 commits June 10, 2024 10:41
Co-authored-by: Joel Oskarsson <joel.oskarsson@outlook.com>
Co-authored-by: sadamov <45732287+sadamov@users.noreply.github.com>
Co-authored-by: sadamov <45732287+sadamov@users.noreply.github.com>
@leifdenby
Copy link
Member Author

I am going to promote this to an actual PR from a draft PR now the discussion is well underway 😄

@leifdenby leifdenby marked this pull request as ready for review June 10, 2024 08:54
@joeloskarsson
Copy link
Collaborator

Any updates on this @leifdenby? Are there specific points you want more input on? Out of my points it is mainly the checkbox for adding tests that requires some further discussion. It would be good to try to merge this soon.

Co-authored-by: Joel Oskarsson <joel.oskarsson@outlook.com>
@leifdenby
Copy link
Member Author

leifdenby commented Jul 7, 2024

TODO

  • remove line about having added tests
  • add option for documentation under the type of change
  • update changelog sections to choose from to be explicitly listed as added, changed, fixed, maintenance

@leifdenby
Copy link
Member Author

Ok, I think this is ready to go now @joeloskarsson. Do you mind giving it a last read over?

@leifdenby leifdenby requested a review from joeloskarsson July 7, 2024 20:00
Copy link
Collaborator

@joeloskarsson joeloskarsson left a comment

Choose a reason for hiding this comment

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

Gave it a proofread and fixed some grammar. After these fixes + updating the docstring text this should be good to merge imo!

leifdenby and others added 5 commits July 9, 2024 16:35
Co-authored-by: Joel Oskarsson <joel.oskarsson@outlook.com>
Co-authored-by: Joel Oskarsson <joel.oskarsson@outlook.com>
Co-authored-by: Joel Oskarsson <joel.oskarsson@outlook.com>
Co-authored-by: Joel Oskarsson <joel.oskarsson@outlook.com>
@leifdenby
Copy link
Member Author

leifdenby commented Jul 11, 2024

Thanks for the thorough review @joeloskarsson!

@leifdenby
Copy link
Member Author

This just needs your approval and then I will merge in :)

Copy link
Collaborator

@joeloskarsson joeloskarsson left a comment

Choose a reason for hiding this comment

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

Great, all looking good now!

@leifdenby leifdenby merged commit a4e9a3b into mllam:main Jul 11, 2024
8 checks passed
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.

4 participants