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

[ENH] Add Commenting on a PR to CONTRIBUTING.md #490

Merged
merged 1 commit into from
Jun 13, 2020

Conversation

franklin-feingold
Copy link
Collaborator

This PR will extend our contributing guide to go over how to submit comments/feedback on pull requests.

Pull requests are our primary method of adding to or extending BIDS. Therefore it is important to have a clear guide for the community to reference. This will further help lower our barriers to entry/participation.

@bids-standard/steering identified this as a need.

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Much of this duplicates https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/commenting-on-a-pull-request. Is that intentional, or did you not know about that?

Also, the first two images are much larger than they need to be.

@franklin-feingold
Copy link
Collaborator Author

franklin-feingold commented Jun 1, 2020

yep that was intentional - The Steering Group wanted a how to comment guide in our contributing guide that is specific to our project.

edit: images have been resized

@sappelhoff
Copy link
Member

Thanks for bringing this up Franklin,

personally I think that linking to existing and maintained information (in the GitHub docs) is preferable to duplicating the information here, which will increase maintenance burden.

That being said, I do see the point that the technical materials may be more accessible when they are tailored to the BIDS repositories (as proposed in this PR). And if the steering group and other community members agree with this perspective, then we should include it.

What are your opinions about this @emdupre @choldgraf ?

Bonus question: Will we run into problems at some point if we keep adding screenshots to the repository (relatively large in size, no sensible "diffs" possible)? Is there an alternative solution?

@choldgraf
Copy link
Collaborator

choldgraf commented Jun 2, 2020

I think that there is value in having this information (particularly if it is high-quality, thanks for this PR!) hosted in a community space such as the documentation, as opposed to relying on the GitHub docs for it. For one, it gives us more control over the kind of behavior that we want to encourage. For another, we should be thankful if someone actually reads this documentation, and the likelihood they will also click through to read generic GitHub "contributing guides" I think is pretty low...

That said, I do think there's value in having one canonical place for this information within a community. E.g., if it's documented here, then other BIDS repos should link to it. Ideally there would be one "BIDS community contributing guide" that all the repos would link to.

re: images and their size, I try to either reduce the image size with something like imagemagick, or make the image small on my screen and take a screenshot of it with the snipping tool to reduce size. Over time it will grow the size of the repo, but not a big deal if the images are really tiny

@emdupre
Copy link
Collaborator

emdupre commented Jun 2, 2020

I'm a strong +1 on having this part of the "GitHub flow" documented; I think this is something that our community members would appreciate additional guidance on.

I agree with @choldgraf that we could avoid duplicating this information across BIDS repositories -- the BIDS Starter Kit is probably the other repo where I'd expect some duplication. But between the two, I'd be more likely to put information here and point to it from the starter kit.

Thanks for thinking on this !

@sappelhoff sappelhoff mentioned this pull request Jun 2, 2020
6 tasks
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.

Okay, thanks for your opinions @emdupre and @choldgraf.

@franklin-feingold I went through the text and left a few suggestions. I think this can be merged even before the 1.4 release, because it does not impact the specification.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@sappelhoff sappelhoff added this to the 1.4.0 milestone Jun 7, 2020
@franklin-feingold
Copy link
Collaborator Author

@sappelhoff thanks for your review - applied the suggestions. I agree - no need to delay this because of the release

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Significant rewording. I tried to unify the voice, reducing the amount of imperative except in the form of "To do X, perform steps A, B and C." I also replaced the backtick quotes to double quotes. Since we're not referring to code, the monospace seemed out-of-place.

I would also drop the BIDS_suggest_preview.png.

This PR should be squashed and merged as a single commit, to avoid having extraneous images in the git history.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@franklin-feingold
Copy link
Collaborator Author

thanks for your review and rewordings @effigies - applied your suggestions and squashed the commits into 1

@sappelhoff
Copy link
Member

This is good to merge from my side: @effigies please merge when you are happy.

@sappelhoff sappelhoff modified the milestones: 1.4.0, 1.4.1 Jun 12, 2020
@effigies effigies merged commit 2859cc2 into bids-standard:master Jun 13, 2020
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.

5 participants