-
Notifications
You must be signed in to change notification settings - Fork 664
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
Update PR template after relicensing #4860
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4860 +/- ##
===========================================
- Coverage 93.65% 93.63% -0.02%
===========================================
Files 177 189 +12
Lines 21779 22861 +1082
Branches 3064 3067 +3
===========================================
+ Hits 20398 21407 +1009
- Misses 929 1002 +73
Partials 452 452 ☔ View full report in Codecov by Sentry. |
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.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
||
Changes made in this Pull Request: | ||
- | ||
- <!-- Describe the changes that this PR makes. --> |
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.
I'd remove, the "Changes made in this Pull Request" is already pretty clear, isn't it?
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.
It is, yet people leave this bullet point empty most of the time... I was an attempt to clarify that they should fill it.
## Developers certificate of origin | ||
- [ ] I certify that this contribution is covered by the LGPLv2.1+ license as defined in our [LICENSE](https://github.com/MDAnalysis/mdanalysis/blob/develop/LICENSE) and adheres to the [**Developer Certificate of Origin**](https://developercertificate.org/). |
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.
I agree with removing.
@IAlibay , your opinion is also important here – can you please weigh in?
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.
I would prefer it if we could keep it please.
The certificate of origin was partly added because we switched the license but also as a long term "you agree that this fits the license and you are the code owner".
To better explain: I use this elsewhere (it's actually a thing that @mikemhenry introduced to me somewhere) and it's not specific to the license switch it's more something we added because it helped and it was kinda useful.
Just to add - my view is open to change, if folks tell me "no one is really checking this box and we hate it" then I guess it's gotta go. But generally it's a nicety that can avoid headaches in future relicensing efforts (ish, it's complicated). |
I am not against the DCO but I also don't understand how it will help us in the future. Can you explain? Maybe I can see it as a guard-rail against people submitting LLM-generated code that is not actually compatible with our license — if/when someone wanted to sue MDAnalysis for including code that we shouldn't have included. The argument in favor of removing the check box is that it makes the PR simpler. Is that enough of an argument against making contributors (at least in principle) aware of their responsibilities? |
So there's a few ways to look at it:
Generally here I'm targetting the "honest actor", which reads this and uses it as an aid / reminder for doing the right thing. Consider it the same as our PR checklist - it's useless for everyone we have to remind to include tests or update the changelog even though the checklist exists, but for the folks that pay attention it's useful. In fact "I checked the license and this code is good for it" is effectively what that box says, so there's a strong argument for saying that it's as relevant as anything else in that checklist (that may or may not get ticked). P.S. note that in my opinion, ticking the box isn't as important as the box existing. edit: LLMs is indeed one of the target areas for this, where you effectively say "well there's a box and we made it your problem to tell me if you think there's an issue, otherwise I won't ask you". |
This was my thinking as well for the removal; I've seen a couple of "can you please tick the box" requests. I don't have strong feelings, I just thought that it is a bit unnecessary and uncommon. If we want to keep it, I'd suggest to remove the explicit mention of the license (it is useless for relicensing anyways) in favor of something about copyright/ownership and make it part of the checklist. What do you think? |
With @IAlibay 's explanation I am tending back towards keeping it, especially for LLM-awareness. If we keep it, I would leave it in it's own section and make clear that this box must be checked or it won't get merged (different from the other boxes). I agree with @RMeli that we can remove the explicit mentioning of the licenses (or we can link to LICENSE as a piece of information). Something like
|
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.
lgtm, thanks @RMeli
Also thank you @IAlibay for the productive discussion! |
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.
Couple of comments but nothing I'd consider blocking.
Enforcing ticking the box might be a "business discucssion" thing? I do generally fall on the "implicit agreement is better than nothing", but I also don't mind either way.
|
||
In order for this PR to be merged you **must certify that you are able to submit your code to be included in MDAnalysis**. Read the links and then check the box if you agree. | ||
|
||
Code in MDAnalysis is under [LICENSE](https://github.com/MDAnalysis/mdanalysis/blob/develop/LICENSE). |
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.
Code in MDAnalysis is under [LICENSE](https://github.com/MDAnalysis/mdanalysis/blob/develop/LICENSE). | |
Code in MDAnalysis is licensed under [LGPLv2+](https://github.com/MDAnalysis/mdanalysis/blob/develop/LICENSE). |
Or something like that? This isn't a "content" comment, but rather a "that sentence flows weird when I read it", so it could just be me.
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.
I think the idea here was to not mention the specific license directly. But maybe it's better to do so. Otherwise I can try to find an alternative phrasing which allows to link to the license file without explicitly mentioning the license.
|
||
## Developers Certificate of Origin | ||
<!-- | ||
CHECKING THE FOLLOWING BOX (BY ADDING 'x' INSIDE THE BRACKETS `[x]`) IS MANDATORY FOR ALL PRs. |
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.
So as per my last comment, I'd be perfectly happy with this being there without a tick box - especially if it's going to be a pain for others.
That is to say, if you wanted to just make this an implicit agreement - i.e. a footer that says "by summiting this code you agree to.." then that'd have the same effect. At the end of the day the good actors will act well and the bad actors will act badly.
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.
Happy to go with a non-ticking implicit agreement. The comment can then say to not delete/modify the following lines one wants the PR to be merged?
|
||
## Developers certificate of origin | ||
- [ ] I certify that this contribution is covered by the LGPLv2.1+ license as defined in our [LICENSE](https://github.com/MDAnalysis/mdanalysis/blob/develop/LICENSE) and adheres to the [**Developer Certificate of Origin**](https://developercertificate.org/). | ||
- [ ] I certify that I can legally submit this code contribution as described in the [**Developer Certificate of Origin**](https://developercertificate.org/) |
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.
This is very much just a nit pick - "legally" can be a bit jarring, the wording can work without too it if you feel like it.
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.
Good point, I'll remove it.
Correct me if I'm wrong, but now that relicensing is completed, can we remove the "Certificate of origin"?
📚 Documentation preview 📚: https://mdanalysis--4860.org.readthedocs.build/en/4860/