Skip to content
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

[MISC] update Contributing with info on how to respond to reviews #655

Merged
merged 9 commits into from
Nov 6, 2020

Conversation

Remi-Gau
Copy link
Collaborator

fixes #648

This should at least give some information about how to use the Github graphic interface to accept suggestions from reviews and how to batch them.

👋 tagging @melanieganz @guiomar for extra feedback on this ( if you have time 😉 ).

For reviewers

  • Is this clear enough?
  • is any information missing?

Possible other changes to add to this PR

Would it make sense to change the order of the section in Contributing so that it maches more the "life cycle" of a PR.

Current order

  • Commenting on a pull request
  • Writing in markdown
  • Making a change with a pull request
  • Example pull request
  • Accepting suggestion from a review

Suggested order

  • Writing in markdown
  • Making a change with a pull request
  • Commenting on a pull request
  • Example pull request
  • Accepting suggestion from a review

@Remi-Gau
Copy link
Collaborator Author

Also can I update my "emoji" in the contributors document? I never know where to squeeze in that commit... 🙈

@sappelhoff
Copy link
Member

I never know where to squeeze in that commit...

you can either make that commit here, or edit this WIKI: https://github.com/bids-standard/bids-specification/wiki/Recent-Contributors

the wiki is synced with the spec once per release.

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Oct 27, 2020

I never know where to squeeze in that commit...

you can either make that commit here, or edit this WIKI: https://github.com/bids-standard/bids-specification/wiki/Recent-Contributors

the wiki is synced with the spec once per release.

Gotcha. 👍

And do you want me to add this ⬆️ to the contributing.md? Could not find it in there.

@sappelhoff
Copy link
Member

And do you want me to add this arrow_up to the contributing.md? Could not find it in there.

That'd be awesome, thanks!

@guiomar
Copy link
Collaborator

guiomar commented Oct 27, 2020

Thanks @Remi-Gau, is there a way to visualize things with the pictures and the final format? Or do people read and suggest comments by reading the green text with the +?

@guiomar
Copy link
Collaborator

guiomar commented Oct 27, 2020

Ok, sorry, forget my comment, I found the button...!! It would have been nice for my past me. Getting used to GitHub :)
I'll delete my comment

@guiomar
Copy link
Collaborator

guiomar commented Oct 27, 2020

Thanks @Remi-Gau!! That's super useful!! I can't see the figures, but I could very well follow the instructions. I think this document with the tips on how to navigate throw here it's really useful!! Thanks a lot :)

@guiomar
Copy link
Collaborator

guiomar commented Oct 27, 2020

Yes, agree with the new suggested order. And also if possible, I think it would be better to enumerate the sections with numbers, it really help to put order and link sequential steps. This happens to me through out all the documentation. I feel that numbers in the headings will really help to have a clear schema of things.

@Remi-Gau
Copy link
Collaborator Author

OK will reorder the sections as suggested.

@Remi-Gau
Copy link
Collaborator Author

I have reordered the document and tried to make sure that some sort of line length was enforced to help with any eventual future reviews.

Sorry if the "diff" looks like a mess because of that.

@melanieganz
Copy link
Contributor

Dear @Remi-Gau, like @guiomar I like your new ordering (even though it was a bit hard from the current diff to read through right now, but I get the gist). So I would say this is a nice addition especially for us that have until now only used Git sporadically. :-) So thanks so much for the clarifications, @Remi-Gau!

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall!

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@franklin-feingold franklin-feingold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! thank you @Remi-Gau !

CONTRIBUTING.md Outdated Show resolved Hide resolved
@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Nov 2, 2020

Maybe for another PR: add some extra links (to the turing-way book) on using github, doing code reviews...

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Remi-Gau are you ready to have this PR merged? Would you first want more people to review it?

@melanieganz @guiomar are you happy? If yes, please "approve" the PR via the GitHub review tool :-)

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Nov 4, 2020

@Remi-Gau are you ready to have this PR merged? Would you first want more people to review it?

Good with me. If I have more ideas, they can be added later. :-)

@effigies
Copy link
Collaborator

effigies commented Nov 6, 2020

Seems to be general agreement that this is an improvement. Let's merge and make further improvements in future PRs.

@effigies effigies merged commit 596a5d6 into bids-standard:master Nov 6, 2020
@Remi-Gau Remi-Gau deleted the remi-update_contributing branch November 10, 2020 05:50
@sappelhoff sappelhoff changed the title update Contributing with info on how to respond to reviews [MISC] update Contributing with info on how to respond to reviews Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update CONTRIBUTING to explain how to "respond to reviews"
6 participants