Add guidelines for PR reviews. #197

smoia · 2020-04-02T18:11:00Z

Closes nothing, but follows #181
I tried to write down some guidelines for PR reviewers (and update those for PR authors), as that process could follow guidelines too.
Before even reviewing it, I'd like @rmarkello @eurunuela @vinferrer @RayStick and the rest of the team to throw in more suggestions and discuss these guidelines.

Proposed Changes

Add review guidelines in contributor guide.
Minor adjustments related to the adoption of auto

codecov · 2020-04-02T18:17:08Z

Codecov Report

Merging #197 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #197   +/-   ##
=======================================
  Coverage   93.68%   93.68%           
=======================================
  Files           8        8           
  Lines         681      681           
=======================================
  Hits          638      638           
  Misses         43       43

RayStick · 2020-04-02T20:13:36Z

I gave it a read and it looks good to me. At the moment, I have one question relating to reviewing and merging PRs:

If I am the last reviewer to approve a PR, but travis still says "some checks haven't been completed yet" I assume I should wait until I merge? Even if the branch has no conflicts with base branch? If so, we should make this clearer in the contributor file.

vinferrer · 2020-04-02T20:27:30Z

Yes you should always wait for the travis test to finish, because if the test fails you may be merging a PR that is breaking the code

smoia · 2020-04-02T21:20:30Z

Yes @RayStick , I added a point about it actually! I pushed, but probably while GitHub was down - I'm trying again.

docs/contributorfile.rst

eurunuela · 2020-04-03T06:54:24Z

I think this is fine. Thanks @smoia !

docs/contributorfile.rst

RayStick · 2020-04-03T15:37:18Z

Yes @RayStick , I added a point about it actually! I pushed, but probably while GitHub was down - I'm trying again.

I see the change now.
Guidelines are clear and easy to read. :)

vinferrer · 2020-04-09T17:25:24Z

looks good to me

RayStick · 2020-04-10T14:49:17Z

@smoia - so are we going with what I suggested in the meeting, that the assignee slot means that person is responsible for checking it gets done, and then doing the final merge?

For the workflow in our lab, I went for these guidelines:

"Use the ‘Reviewer’ option to assign one (or more) people to review the changes you have made. If the ‘Assignee’ option is left blank, the last reviewer to approve the PR should do the final merge. If you don’t want this to be the case, use the ‘Assignee’ option to specify which person you want to merge the new branch into master.

If we want something similar, we should add some text explaining this in the docs, and I can suggest some. But if anyone has different thoughts on that, let me know.

smoia · 2020-04-10T16:26:18Z

@RayStick yes, I added them in the last commit.
The contributor guidelines document has become huge and probably too complex to be read, if anyone has suggestion on how to improve it, I'd welcome new and fresh ideas!

RayStick · 2020-04-10T17:20:56Z

I can read it properly next week, and try to cut it down a bit, if you like.

smoia · 2020-04-10T19:00:35Z

@RayStick sure, thank you!
I don't know if it's about cutting or about reorganising it, maybe using the other contributor-targetting page, this one

smoia · 2020-05-14T16:49:02Z

@RayStick I'm leaving this as a Draft due to your proposal of rewriting it. Would you have time to do so soon? If not, I'm going to graduate it to PR ready for review and if it's accepted we'll merge it in as it is - we can always work on it at another moment.

RayStick · 2020-05-14T17:17:15Z

I will read it today! Anything specific you want feedback on or improved, or is it just a general read for clarity?

smoia · 2020-05-14T18:38:26Z

I can read it properly next week, and try to cut it down a bit, if you like.

@RayStick I was referring to this post. It's not necessary short term though! If you think that the documentation is clear and can be merged as is, don't worry about it - just merge it in and we'll think about how to reorganise it at a later time!

eurunuela · 2020-05-15T11:42:33Z

@smoia, @RayStick it looks like you have some merge conflicts. How close from review do you think this is?

RayStick · 2020-05-15T13:59:34Z

I'll resolve them now

RayStick · 2020-05-15T14:11:39Z

@eurunuela We are very close to it needing review, but I just want to check a few things with @smoia first.

RayStick · 2020-05-15T19:23:55Z

@smoia I am making a few more smaller changes, that I will submit to your local branch soon.

RayStick · 2020-05-18T19:08:38Z

@smoia let me know if anything is unclear with my most recent changes to these documents. After they've been looked at, then it is ready for review by others.

Text Re-arrangement

smoia · 2020-05-18T20:31:18Z

I'm sorry @RayStick , I just missed the PR. @eurunuela do you want to be the main reviewer for this one?

RayStick · 2020-05-18T20:41:29Z

No problem @smoia !

eurunuela · 2020-05-19T06:57:37Z

I'm sorry @RayStick , I just missed the PR. @eurunuela do you want to be the main reviewer for this one?

Sure, I'll take the lead!

eurunuela

LGTM! Thanks for the changes @smoia and @RayStick !

docs/contributorfile.rst

eurunuela

LGTM! Thanks for the changes @smoia and @RayStick !

eurunuela · 2020-05-27T07:17:08Z

Can you please have a look at this @vinferrer and PR?

eurunuela · 2020-06-01T09:41:13Z

Given that this is a documentation change and only one PR is needed, I'll merge if @smoia and @RayStick are happy with it.

smoia · 2020-06-01T09:46:27Z

I'm puzzled by the loss of coverage 🤔.
In any case, this is a paused PR, since it kind of depends on #181 being merged first.
#181 will be merged as soon as we close #206, #212, #222, #223 - it should happen in two weeks!
If it's accepted, I'll merge it in at the right time. Thank you for keeping an eye on it though!

eurunuela · 2020-06-01T09:47:57Z

I didn't know this PR depended on #181 . Good to know!

Add review guidelines in contributor guide.

7c77635

smoia added Documentation This issue or PR is about the documentation Paused Issue should not be worked on until the resolution of other issues or Pull Requests Community This issue is about the community, not the code labels Apr 2, 2020

Add a couple of reviews points

56a8117

Trigger update

5187620

eurunuela reviewed Apr 3, 2020

View reviewed changes

docs/contributorfile.rst Outdated Show resolved Hide resolved

vinferrer reviewed Apr 3, 2020

View reviewed changes

docs/contributorfile.rst Outdated Show resolved Hide resolved

Minimal text tweak, add more specifications

3182974

smoia assigned RayStick Apr 10, 2020

Add main reviewer

0135265

RayStick requested review from RayStick and removed request for RayStick May 14, 2020 17:24

smoia marked this pull request as ready for review May 14, 2020 18:36

Rachael Stickland added 2 commits May 14, 2020 14:01

layout and wording

0adeb8c

corrections

b5754e9

Merge branch 'master' into doc/reviews

c9769e0

RayStick requested review from RayStick and removed request for RayStick May 15, 2020 19:01

Text rearrangment

3e6cb71

Merge pull request #3 from RayStick/smoia/doc/reviews

334aca4

Text Re-arrangement

smoia unassigned RayStick May 18, 2020

Trigger CI

b8dc51c

smoia mentioned this pull request May 25, 2020

Add BIDS-compatible automatic reformatting of units of measure #222

Merged

eurunuela approved these changes May 27, 2020

View reviewed changes

docs/contributorfile.rst Outdated Show resolved Hide resolved

docs/contributorfile.rst Outdated Show resolved Hide resolved

eurunuela approved these changes May 27, 2020

View reviewed changes

Add Azure link to CI text

263e082

smoia assigned eurunuela May 27, 2020

Merge remote-tracking branch 'upstream/master' into doc/reviews

e826afe

eurunuela mentioned this pull request Jun 9, 2020

Add the possibility to split multi-run physiological recordings #206

Merged

17 tasks

smoia mentioned this pull request Jun 15, 2020

Update testing to use trimmed files #226

Closed

Merge branch 'master' of github.com:physiopy/phys2bids into doc/reviews

88d09cc

smoia merged commit f6fc624 into physiopy:master Jun 15, 2020

smoia added the released This issue/pull request has been released. label Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add guidelines for PR reviews. #197

Add guidelines for PR reviews. #197

smoia commented Apr 2, 2020

codecov bot commented Apr 2, 2020 •

edited

Loading

RayStick commented Apr 2, 2020 •

edited

Loading

vinferrer commented Apr 2, 2020

smoia commented Apr 2, 2020

eurunuela commented Apr 3, 2020

RayStick commented Apr 3, 2020

vinferrer commented Apr 9, 2020

RayStick commented Apr 10, 2020 •

edited

Loading

smoia commented Apr 10, 2020

RayStick commented Apr 10, 2020

smoia commented Apr 10, 2020 •

edited

Loading

smoia commented May 14, 2020

RayStick commented May 14, 2020

smoia commented May 14, 2020

eurunuela commented May 15, 2020

RayStick commented May 15, 2020

RayStick commented May 15, 2020

RayStick commented May 15, 2020

RayStick commented May 18, 2020

smoia commented May 18, 2020

RayStick commented May 18, 2020

eurunuela commented May 19, 2020

eurunuela left a comment

eurunuela left a comment

eurunuela commented May 27, 2020

eurunuela commented Jun 1, 2020

smoia commented Jun 1, 2020

eurunuela commented Jun 1, 2020

Add guidelines for PR reviews. #197

Add guidelines for PR reviews. #197

Conversation

smoia commented Apr 2, 2020

Proposed Changes

codecov bot commented Apr 2, 2020 • edited Loading

Codecov Report

RayStick commented Apr 2, 2020 • edited Loading

vinferrer commented Apr 2, 2020

smoia commented Apr 2, 2020

eurunuela commented Apr 3, 2020

RayStick commented Apr 3, 2020

vinferrer commented Apr 9, 2020

RayStick commented Apr 10, 2020 • edited Loading

smoia commented Apr 10, 2020

RayStick commented Apr 10, 2020

smoia commented Apr 10, 2020 • edited Loading

smoia commented May 14, 2020

RayStick commented May 14, 2020

smoia commented May 14, 2020

eurunuela commented May 15, 2020

RayStick commented May 15, 2020

RayStick commented May 15, 2020

RayStick commented May 15, 2020

RayStick commented May 18, 2020

smoia commented May 18, 2020

RayStick commented May 18, 2020

eurunuela commented May 19, 2020

eurunuela left a comment

Choose a reason for hiding this comment

eurunuela left a comment

Choose a reason for hiding this comment

eurunuela commented May 27, 2020

eurunuela commented Jun 1, 2020

smoia commented Jun 1, 2020

eurunuela commented Jun 1, 2020

codecov bot commented Apr 2, 2020 •

edited

Loading

RayStick commented Apr 2, 2020 •

edited

Loading

RayStick commented Apr 10, 2020 •

edited

Loading

smoia commented Apr 10, 2020 •

edited

Loading