arfc / arfc.github.io

Holds the research group website.
Creative Commons Attribution 4.0 International
12 stars 62 forks source link

Additional point under Enhancing Clarity #319

Closed RhysMacMillan closed 2 months ago

RhysMacMillan commented 2 months ago

Summary of changes

The word significant offers judgement when used outside the context of statistical significance. The writing course emphasizes this point and the current arfc writing checklist omits it.

Types of changes

Associated Issues and PRs

Associated Developers

Checklist for Reviewers

Reviewers should use this link to get to the Review Checklist before they begin their review.

nsryan2 commented 2 months ago

After you commit changes, you can re-request a review from me by clicking the arrows next to my name in the reviewer list.

Normally, this kind of PR I would review and it would say accept, but I would still want you to address my minor comment before it gets merged. I didn't do that this time just so it was very clear what was going on.

You'll see this when you start reviewing PRs, but there are three options when you start a review. How I think of them is:

  1. accept (I use this for PRs that are most of the way or all of the way there),
  2. comment (PRs that are pretty good, but need some changes),
  3. and request changes (PRs that need some serious re-working, where I wouldn't like it to be merged in its current form).
nsryan2 commented 2 months ago

LGTM (looks good to me)! The last thing I'll ask is that you add labels and a project (choose the meta project) to this PR, and then I'll merge.

Edit: It's always good to—when you are reviewing a PR—say explicitly what you would like to be changed or what is blocking you from merging the PR so the assignees can best address your feedback. You'll notice that I say specifically why I am not merging the PR.

The labels and projects might seem like a small detail, but it all adds up overtime so it's good to be on top of things from the start.