Add code reviewing info to contributors guide

[handwerkerd]

Summary

As I'm doing a bit more code reviews for pull requests, I'm finding myself searching for the same things over & over and fumbling a bit. It would be helpful to have a section in the contributors' guide on how to review a pull request.

Additional Detail

Here are some things that would be nice to include. Hopefully, many of these can just be a framework of text that links to existing open resources.

• [ ] How to pull a PR for local testing and review. (Similar to: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/checking-out-pull-requests-locally )
• [ ] How to submit a PR to a PR or otherwise suggest code edits to a PR
• [ ] If testing fails, how to understand/trace the error messages.
• [ ] How to save/locate the files generated when pytest is run to more directly view any changes in program outputs.
• [ ] Style guide for reviewing
• [ ] Info on what makes a review useful: What should hold up approving a PR vs be dealt with in another issue?
• [ ] Perhaps more for a developer discussion than the contributors' guide, but any general rules for when a reviewer should or should not approve a PR that breaks a test? Is the approval of two reviewers sufficient or should those inherently involve a broader amount of discussion?

Next Steps

• If people have good links for these issues or other related things to put in the documentation, add them below.
• I'm tagging this as a good first issue since someone who is an experienced developer, but knows nothing about tedana could potentially do this.

[handwerkerd]

May be of interest to @KirstieJane @emdupre @rmarkello @jbteves @tsalo

[jbteves]

Something that occurs to me as we continue experiencing these problems is that they're increasingly not unique to tedana. I'm not sure what forum would be good to put these in, but this is getting to be very general information, with the exception of project-specific decisions like approving vs. waiting for a new issue. I'm sure @KirstieJane and @emdupre have some thoughts on where we draw the line. I've added some basics for git to the guidelines just because it's really hard to find them in related projects and those actions are essential to contributing. Do you have any thoughts on whether tedana itself needs some of these pieces of information to be part of the project instead of offered in a more general platform, @handwerkerd ?

[handwerkerd]

Like I said, I'm hoping most of this info is already available elsewhere, but probably not in one place. If it's already in one place, we should link to it! Assuming it doesn't exist yet, centralizing and contextualizing existing resources would definitely be useful well beyond tedana.

For a person who has the time, interest, & knowledge, creating a stand-alone resource or just creating it in tedana are both options.

My general sense is, if one wants to make a stand-alone guide, there will be a bigger start-up time cost before it's worth referencing. My suspicion is that such a general guide is more likely to become a reality if we let the tedana contributors' guide grow until someone decides it's worth the time to fork it into a stand-alone repository. The key would be to expand the tedana guide with this future fork in mind. That said, as the person not doing this work, I'd be overjoyed at whichever approach whoever does do the work prefers.

In the meantime, if there are good existing links, add them to the comments here. That might give a better sense if this specific issue is a big ask or could be accomplished with a paragraph & a few links.

[tsalo]

@smoia recently posted this interesting blog post in the phys2bids Gitter channel. I read it and think it is pretty clear and concise. It covers a lot of the same ground that we already have, but there were a couple of points that might be worth adding here.

First, the author recommends automated code formatting and automated documentation checking. We already have issues for these (#219 and #527, respectively), but it's worth noting that these are apparently broadly accepted automated steps.

Second, the author proposes including checks for "I ran this code locally" and "I wrote the necessary tests" in the PR template. These would be easy enough to add and could help with code review.

[eurunuela]

Second, the author proposes including checks for "I ran this code locally" and "I wrote the necessary tests" in the PR template. These would be easy enough to add and could help with code review.

I loved this part. I think it can make the reviewer's life easier and is super easy to implement/use.

[emdupre]

The Turing Way also has a chapter on reviewing pull requests that might be useful to link to ! https://the-turing-way.netlify.app/collaboration/github/github-maintainers.html