-
Notifications
You must be signed in to change notification settings - Fork 659
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
[WIP] Fix authors & changelog #4361
Conversation
Linter Bot Results:Hi @IAlibay! Thanks for making this PR. We linted your code and found the following: There are currently no issues detected! 🎉 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4361 +/- ##
==========================================
Coverage 93.37% 93.37%
==========================================
Files 170 184 +14
Lines 22340 23453 +1113
Branches 4085 4085
==========================================
+ Hits 20859 21899 +1040
- Misses 963 1036 +73
Partials 518 518 ☔ 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.
LGTM :)
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 it is "ok" to put the small doc improvements in, CI is happy + core dev approval. I guess there's a WIP in the title, but can always open another if you find more.
I should have made this into a draft.. this was still WIP 😅 |
for context in why it was WIP - @orbeckst wasn't sure if we should be adding authors without asking them first, plus we're reaching far into history |
@orbeckst do you want to weigh in here if you want to revert this change? |
They're already in the |
It's a bit late on my end, but iirc one of Oliver's concerns is that he thinks he might have been the one to add at least one of them (i.e. by tagging them in on a commit). Not really sure I can accurately recollect that discussions - it's probably ok. |
Ok, let's ask them if they mind being listed as authors in MDAnalysis logs: cc @dspoel @bsipocz @mimischi I'll purge them out if they don't want to be listed, the rest of the changes are valid fixes I think. Defaulting to giving credit if you scrape the author list makes sense to me at least. Adding names to published papers is another matter I suppose, but that's different. |
Thanks @tylerjereddy 🙏 |
@@ -99,7 +99,7 @@ Changes | |||
Deprecations | |||
|
|||
|
|||
08/15/23 IAlibay, jaclark5, MohitKumar020291, orionarcher, xhgchen, | |||
15/08/23 IAlibay, jaclark5, MohitKumar020291, orionarcher, xhgchen, |
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.
That's not a legal MM/DD/YY — I think this needs to be reversed to 08/15/23.
(Sorry that we used US convention... should have used ISO YYYY-MM-DD)
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.
Ahhh I think I messed up and changed the date in the wrong place sorry! Should have been line 79 🤦🏽♂️ teaches me to not pay attention.
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'm confused, I based my review on the date above this one, which is 28/08/23
. Are there problems in multiple locations then?
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.
@tylerjereddy it's the one above that is wrong (that's what I was meaning to fix 😅 and changed the wrong one because I was in a rush) - so yes now there are two wrong dates... (sorry)
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.
Ah, I see, I guess Irfan did the same thing maybe, most are the other format.
@@ -42,6 +42,7 @@ Chronological list of authors | |||
- Lukas Stelzl | |||
- Jinju Lu | |||
- Joshua L. Phillips | |||
- David van der Spoel |
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.
* Add bsipocz from PR MDAnalysis#1408 * David van der Spoel from commit 54ed4b3 * gh seems to indicate a 2012 merge (commit may have been circa 2010?) * Add mimischi from PR 2356 * Fix date for 2.6.1 and duplicate authors in changelog
* I merged MDAnalysisgh-4361 a bit early, so I should clean up the mess; I think this fixes the two cases where the datetime string was not formatted to match our documented convention [skip cirrus]
* I merged gh-4361 a bit early, so I should clean up the mess; I think this fixes the two cases where the datetime string was not formatted to match our documented convention [skip cirrus]
Various fixes as I cross-check through the history of our contributors.
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4361.org.readthedocs.build/en/4361/