search

nuest / ten-simple-rules-dockerfiles

Ten Simple Rules for Writing Dockerfiles for Reproducible Data Science
https://doi.org/10.1371/journal.pcbi.1008316
Creative Commons Attribution 4.0 International
61 stars 15 forks source link

Include feedback from OSF (Hypothesis), use CRediT taxonomy for contributions, includes editorial review #68

Closed nuest closed 4 years ago

vsoch commented 4 years ago

@nuest aren't some of these changes done by @benmarwick ? Why don't we see his commits here, and you are listed as the author (e.g., see blame view https://github.com/nuest/ten-simple-rules-dockerfiles/blame/390f259e41cc5658383bec39e7c9aebf0a52a8f1/ten-simple-rules-dockerfiles.Rmd#L123?)

vsoch commented 4 years ago

For the reviewer, it would be good to maintain his commits, and then make your changes on top of that (with different commits) otherwise it's impossible to see what you've changed, and the only possible way to review is to re-read everything. Are you able to adjust this PR to properly reflect his contributions, and show your changes cleanly in separate commits?

nuest commented 4 years ago

@vsoch Maybe you mean @bdevans ? I only did some reformatting on his changes (adding new lines) in https://github.com/nuest/ten-simple-rules-dockerfiles/pull/68/commits/16442f16e861acdbdc142c7b606a204791e7398a

Should I re-do those in a specific commit?

vsoch commented 4 years ago

oops yes wrong GitHub alias :) yes that's the right b-starting alias I meant to say!

nuest commented 4 years ago

OK - with the right Ben mentioned - I don't know how what to do differently about the change to @bdevans's additions.

vsoch commented 4 years ago

@nuest the commit I'm looking for is https://github.com/nuest/ten-simple-rules-dockerfiles/pull/64 - is it still in there? I'm wondering because I'm seeing blocks of text (e.g., the figure caption, which I remember reviewing) that are marked as new here.

nuest commented 4 years ago

I only changed one word in the figure caption, maybe the online diff does not catch that well because it is a very lon line, so here's a screenshot from my local git tool:

image

The PR was merged in https://github.com/nuest/ten-simple-rules-dockerfiles/commit/74c7d2c9a3adcf46fce5e4cce8e98116d992714a

vsoch commented 4 years ago

Can you walk me through how you merged it (so why your GitHub alias has credit for the commit?) Was it a squash and merge in the interface (and did you add yourself as a co-author?) It's okay if there was a bit of mess up - but I do think it's important that contributors are given proper credit for their work. think we could rectify it here if we add a comment to the merge message "Co-Authored by" and then put @bdevans email. see https://help.github.com/en/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors.

vsoch commented 4 years ago

okay I think we are probably okay then - it must be that you changed around that paragraph enough for it to be fully green again. I just chose (another) random line from that PR and @bdevans gets the correct attribution:

image

I'll continue with my review, just wanted to be careful about these things! :)

nuest commented 4 years ago

If you go back one step in the blame, then the commit is by @bdevans : https://github.com/nuest/ten-simple-rules-dockerfiles/blame/93568a7222530ded90e4eca1b0cba65119e33772/ten-simple-rules-dockerfiles.Rmd#L126

nuest commented 4 years ago

Thanks for being careful! I was just talking to someone about the "tracking contributions" aspect of writing papers on GitHub, and why I'm not expecting it, it is good to know that someone could trace back individual contributions.

nuest commented 4 years ago

@vsoch Can you wait 10 minutes with your review? Then I'll push some more review-worthy changes.

nuest commented 4 years ago

@vsoch What do you think about removing Kitematic because it's depracted?

In https://hyp.is/GJYCroDZEeqMIncvK264wg/osf.io/fsd7t/ image

vsoch commented 4 years ago

I don't use a Mac so I don't (or didn't) use Kinematic, but if it's reported to be deprecated I agree it should be removed.

nuest commented 4 years ago

@vsoch I hope I could improve the text based on your feedback. I think we made some good improvements on clarity, and might even have added a useful new idea or two.

nuest commented 4 years ago

@vsoch @benmarwick @sje30 @betatim @psychemedia @bdevans

This is an update addressing the comments we had on OSF, consistent use of language, missing formal requirements (see #24), and a change of the contribution section to use the CRediT Taxonomy.

Please everyone take a look at the updated contribution section. Ideall you leave a quick comment on https://github.com/nuest/ten-simple-rules-dockerfiles/issues/69 that all in in order.

Also, the "author summary" is a first attempt that would benefit from one/two second pairs of eyes. Thanks!

@bdevans Please carefully check your affiliation in the Rmd header. Thanks for your contribution! :tada:

nuest commented 4 years ago

@vsoch I' like to merge this to avoid precisely looking at previous changes. I'll resolve your final comments asap, let me know when you're done.

https://github.com/nuest/ten-simple-rules-dockerfiles/pull/65#discussion_r420388833 also points to you and @bdevans thinking there's content that could be cut, so we should get master into a good state.