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

[DOC] Adds 'quick start' guidelines for new contributors #293

Merged
merged 10 commits into from
May 30, 2019
Merged

[DOC] Adds 'quick start' guidelines for new contributors #293

merged 10 commits into from
May 30, 2019

Conversation

jbteves
Copy link
Collaborator

@jbteves jbteves commented May 22, 2019

Closes #292 .

Changes proposed in this pull request:

  • Changes CONTRIBUTING and README to give a new contributor "quick start"

@jbteves
Copy link
Collaborator Author

jbteves commented May 22, 2019

Sorry to ask for so many reviewers but if we're successful at OHBM there will be a lot of eyeballs on this; therefore, I'd like more eyeballs on it sooner rather than later!

@jbteves jbteves requested a review from dowdlelt May 22, 2019 18:39
@jbteves jbteves changed the title Adds 'quick start' guidelines for new contributors [DOC] Adds 'quick start' guidelines for new contributors May 22, 2019
@jbteves jbteves mentioned this pull request May 22, 2019
3 tasks
@codecov
Copy link

codecov bot commented May 22, 2019

Codecov Report

Merging #293 into master will decrease coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #293      +/-   ##
=========================================
- Coverage   49.36%   49.2%   -0.17%     
=========================================
  Files          39      39              
  Lines        2133    2140       +7     
=========================================
  Hits         1053    1053              
- Misses       1080    1087       +7
Impacted Files Coverage Δ
tedana/selection/tedica.py 23.52% <0%> (-1.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84b7c4c...390a124. Read the comment docs.

Copy link
Collaborator

@dowdlelt dowdlelt left a comment

Choose a reason for hiding this comment

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

If I'm going to be picky I would like it to say "Let us know what your interests are ..." rather than 'skills'. I had no skills when I came to tedana but I was really really interested in seeing some figures for the results so thats what I dove into.

@jbteves
Copy link
Collaborator Author

jbteves commented May 23, 2019

Pickiness is great.

I don't know if you knew, but you can mark your review as "request changes" when you do a review, which makes it more explicit that you'd like a change. It also prevents merging.

@emdupre
Copy link
Member

emdupre commented May 26, 2019

So sorry, adding the contributors section added a merge conflict ! Is there any way you could patch that ?

@jbteves
Copy link
Collaborator Author

jbteves commented May 26, 2019

Okay, I think that's resolved the way we'd like. Let me know if not.

README.md Outdated
@@ -82,7 +82,10 @@ Open or comment on one of [our issues](https://github.com/ME-ICA/tedana/issues)!

We ask that all contributions to ``tedana`` respect our [code of conduct](https://github.com/ME-ICA/tedana/blob/master/CODE_OF_CONDUCT.md).

## Contributors
Copy link
Member

Choose a reason for hiding this comment

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

I think we still want this heading, just below the quick start recommendation 😸

Also, WDYT about moving up this text to above the code of conduct reminder ? That way it's clear that the CoC governs gitter, too. At least, that's how I'd read it !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow, I totally missed this comment until @tsalo pointed it out, my bad. The subtraction was another merge casualty; I really need to be more careful next time.

What would you think about additionally adding a line to clarify that the CoC applies broadly across the project, just to be super-duper clear? I (optimistically) hope it never becomes an issue, but at least then it's clear.

tsalo
tsalo previously approved these changes May 30, 2019
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

I agree with @emdupre's point, but other than that it looks good to me.

@jbteves jbteves requested review from emdupre and tsalo and removed request for emdupre May 30, 2019 14:43
@jbteves
Copy link
Collaborator Author

jbteves commented May 30, 2019

I've addressed the above, and added more broad applicability to the CoC statement. I can revert the commit or adjust it if it's not what you'd like.

README.md Outdated Show resolved Hide resolved
@tsalo tsalo self-requested a review May 30, 2019 14:54
tsalo
tsalo previously approved these changes May 30, 2019
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@emdupre emdupre left a comment

Choose a reason for hiding this comment

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

LGTM, too ! Thanks !!

@jbteves jbteves merged commit 65f89e1 into ME-ICA:master May 30, 2019
@jbteves jbteves deleted the JT_add_contributor_quickstart branch May 30, 2019 17:37
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.

Add New Contributor quick start
4 participants