-
Notifications
You must be signed in to change notification settings - Fork 156
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] Update contributing guide and README to make discussion forums easy to find #279
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think this is a great improvement and still taking into account Franklin's and my points in #273 and #275
While we are at it, could we perhaps delete the following lines from the CONTRIBUTING.md?
- The "We're so excited you're here and want to contribute." at the beginning
- The "Thank you!" and "You're awsome" at the end
I think these are slightly distracting :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say this looks good. A couple small comments.
CONTRIBUTING.md
Outdated
|
||
## Contributing through GitHub | ||
|
||
[Git](https://git-scm.com/) is a really useful tool for version control. [GitHub](https://github.com/) sits on top of git and supports collaborative and distributed working. | ||
[git](https://git-scm.com/) is a really useful tool for version control. [GitHub](https://github.com/) sits on top of git and supports collaborative and distributed working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if there's an MLA convention or something, but I would capitalize at the beginning of a sentence, unless using backticks to indicate that we're referring specifically to the CLI git
.
README.md
Outdated
When you're ready to get started, check out [our contributing guidelines](https://github.com/bids-standard/bids-specification/blob/master/CONTRIBUTING.md). | ||
|
||
Want to learn more about working with BIDS? Have a question, comment, or suggestion? | ||
Open or comment on one of our [NeuroStars issues](https://neurostars.org/tags/bids)! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a little out of place in the "Contributing section" and might be skipped over by a new user that doesn't consider themselves an aspiring contributor. Perhaps it would be better to place it as the lat thing before the sub-section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved and added mention of the Starter Kit based on a previous comment from @franklin-feingold
Thanks for the feedback, @sappelhoff and @effigies ! Re :
Personally I quite like the intro-outro, but I'm happy to be outvoted if you feel strongly. |
@@ -1,13 +1,28 @@ | |||
[![Build Status](https://travis-ci.com/bids-standard/bids-specification.svg?branch=master)](https://travis-ci.com/bids-standard/bids-specification) | |||
[![CircleCI](https://circleci.com/gh/bids-standard/bids-specification.svg?style=svg)](https://circleci.com/gh/bids-standard/bids-specification) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding the twitter badge in
[![@BIDSstandard](http://img.shields.io/twitter/follow/bidsstandard.svg?style=social)](https://twitter.com/BIDSstandard) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appears #268 takes care of adding the badge. added this to ensure the version consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't commit this suggestion, then, so I don't create a merge conflict ! Let me know if I'm misunderstanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can keep this hear in case it is needed. It depends on the order of merging since we have a number of PRs open with differing versions of text. I may be mistaken too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine #268 will merge first since I have zero approving reviews (having successfully dismissed my previous two 😅 ). I'll just merge in master if something breaks when it's merged !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll just merge in master if something breaks when it's merged !
yep, that's the way to go :-) (or even better: git rebase master
from your branch, because then we won't have a "merge commit" --> these are often making the history a bit dirty I find)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one caveat though is this will cause the changelog to go out of sync (until a merge commit is seen then it will catch up). the merge commit message is the signal for the changelog generator to generate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks great! Thank you @emdupre for putting this together!
+1, I like it too. I think it helps with the community aspect of the document |
Co-Authored-By: Franklin Feingold <35307458+franklin-feingold@users.noreply.github.com>
yes I think this is really a question of taste, perhaps we can find a middle ground?
If we want something to "round off" the contributing guide, how would you feel about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm extremely neutral on the bubbly intro/outro, so I am approving based on the current modifications.
To clarify - the proposal would remove
and replace with "Welcome to the BIDS contributing guide!" ?
We can thank those that are interested and made it through the contributing guide. I understand not wanting to overdo on thank yous, but this helps create the warm and welcoming community we want to support and grow.
The contributing guide can be a pep talk to help those get to the contributing stage. Contributing can be a stressful time and relieving those concerns is important to open the way for the first contribution and many more. IMO if you are a contributor or want to I think you are awesome :) With that being said and finding a middle ground - I can agree with second point. On the first and third point, I think starting and ending the document with something warm and friendly is really good and gives way to positive community building. As you said, this is a discussion and question of taste. |
Okay, let's hear what @emdupre has to say about this. I would be fine with a middle ground 👍 |
What about having the intro as:
And the outro as:
|
I think those are fine... That said, I'm a little weirded out by the dynamic here, where improving the accessibility of information has become a minor conflict over the tone of text that's not actually within the suggested changes. If we do want to change it, I'm coming to feel like it should be a separate discussion. People who saw this PR initially will have made an impression of what it addresses, which is overall more content than tone, and decided whether they want to weigh in. By expanding the scope, we're bypassing that informal process on a tone change that people might have strong opinions on, even if they didn't have strong opinions here. |
I also think the suggestion is good @emdupre And I agree with @effigies that this has gotten a bit out of hand ... that was not my intent when I started this in #279 (review) Sorry :-) |
@sappelhoff I suggest that we merge this, since the existing changes here seem uncontroversial. Then, if you like you can open a PR with the suggested tone changes, and we can have that discussion, there? |
Agreed |
I agree with @emdupre changes. Agree and I'm sorry that this has progressed to this point. I agree with a separate thread/PR to discuss tone |
For consideration to update text in the README and CONTRIBUTING documents.
Related to #273, #275.
Pinging @franklin-feingold @sappelhoff @effigies based on previous conversations, but all feedback is welcome !
Rendered README.
Rendered CONTRIBUTING.