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] Request for Comments: Roadmap and Contributing #151

Merged
merged 32 commits into from
Nov 13, 2018

Conversation

emdupre
Copy link
Member

@emdupre emdupre commented Nov 1, 2018

Closes #131, #148.

Changes proposed in this pull request:

This is a request for comments on tedana's vision and development philosophy ! Special thanks to @KirstieJane for helping me with the text and for integrating the suggested milestones into the GitHub repository ✨

@ME-ICA/tedana-devs, it would be especially great to get your feedback, and comments from all community members are welcome !

@emdupre emdupre added this to the healthy community milestone Nov 1, 2018
@emdupre emdupre added documentation issues related to improving documentation for the project community issues related to building a healthy community labels Nov 1, 2018
@tsalo
Copy link
Member

tsalo commented Nov 1, 2018

I added a comment to #146 relating to the BIDS compatibility point included in the Roadmap, but other than that everything looks good to me.

@codecov
Copy link

codecov bot commented Nov 1, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #151   +/-   ##
=======================================
  Coverage   48.72%   48.72%           
=======================================
  Files          32       32           
  Lines        2079     2079           
=======================================
  Hits         1013     1013           
  Misses       1066     1066

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 8f34d3a...d6a3c67. Read the comment docs.

@emdupre
Copy link
Member Author

emdupre commented Nov 1, 2018

Thanks, @tsalo !! I just revamped the RTD contributing guidelines to address our high-level contributing philosophy and governance structure. I'm about to push one more PR with updates to the CONTRIBUTING.md file to address all practical aspects of contributing. It'd be great to get your feedback on both of these, too !

I just noticed your #150 PR -- that would be perfect to add in to this since right now I'm linking just to the CONTRIBUTING file for where users / developers can go for support, which is less than ideal. Can I help you in any way with that PR ?

@emdupre emdupre changed the title [DOC] Request for Comments: Roadmap [DOC] Request for Comments: Roadmap and Contributing Nov 1, 2018
@tsalo
Copy link
Member

tsalo commented Nov 1, 2018

I think we just need to resolve the issues with #142 and merge that before we merge #150. I think that #150 will cause merge conflicts in #142 and I don't want to make a new contributor deal with that. Other than that I think it's ready to review.

@emdupre
Copy link
Member Author

emdupre commented Nov 1, 2018

Ok, sounds great. We can have that conversation in #150, but do let me know what you think of the new contributing philosophy, and the updated contributing guidelines !

Copy link

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Wow, quite extended.. left minor remarks from a quick pass over

CONTRIBUTING.md Outdated
* **[TST]** for new or updated tests
* **[DOC]** for new or updated documentation
* **[STY]** for stylistic changes
* **[RF]** for refactoring existing code

Choose a reason for hiding this comment

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

They describe orthogonal dimensions, and thus quite often you want to combine multiple, eg [FIX(TST)] to signal a fix in the tests, or [FIX+TST] to signal a fix accompanied with a test (at least this is alongside the convention I use). So may be too would like to limit to not encourage this, or to allow and explain ;-)

Another one we rarely but do use is BK for commit which breaks build or tests (could come handy while bisecting)

Also we use TEMP or WIP for a commit which sleeping still be rewritten (eg to provide adequate description or squash multiple). There is even a little wip bot service for GitHub to mark pr as failing as long as commits have that in a commit message

Copy link
Member

Choose a reason for hiding this comment

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

There's a paragraph below that mentions WIP, but I think it would be clearer if WIP was included in the bulleted list.

Also, on an unrelated note (but unfortunately linked to this exact line), I would like to request that we change RF to REF. Everything else is three letters, so I think it would look better.

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of adding [WIP] to the list and totally fine with [REF] instead of [RF].

I also like the combinations! @emdupre, @tsalo - what format do you both currently use? I think I've seen commas between the different categories, right? Eg: [WIP, DOC, TST]?

Choose a reason for hiding this comment

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

FWIW, we don't bother having [] decoration and just add : at the end in the datalad https://github.com/datalad/datalad/blob/master/CONTRIBUTING.md#how-to-contribute
Saves us an entire character

docs/roadmap.rst Outdated
Moving forward, we want to grow an active development community,
where developers feel empowered to explore new enhancements to the ``tedana`` code base.

One means to ensure that new code does not introduce silent failures is through extensive testing.

Choose a reason for hiding this comment

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

Silent failures... Not sure what those are. May be just say bugs? Could be clarified (leading to incorrect computation, failing execution, etc)


Pull Requests
`````````````
As is stated in the code, severe or repeated violations by community members may result in exclusion

Choose a reason for hiding this comment

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

Suggested change
As is stated in the code, severe or repeated violations by community members may result in exclusion
As it is stated in the code, severe or repeated violations by community members may result in exclusion

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow the suggested change here - but I think the fact that @yarikoptic doesn't read the sentence the same way I do is clear that it needs a rework!

I think closer to what I was intending is:

Suggested change
As is stated in the code, severe or repeated violations by community members may result in exclusion
As stated in the code, severe or repeated violations by community members may result in exclusion

@tsalo
Copy link
Member

tsalo commented Nov 2, 2018

I like the updated contributing guidelines as well. 👍 overall for the PR.

@emdupre
Copy link
Member Author

emdupre commented Nov 2, 2018

Thanks so much for the feedback, @yarikoptic !

It is a lot of text -- we're hoping that by being very explicit about these ideas in the beginning, it will be easier for new contributors to hit the ground running 🏃‍♂️

@KirstieJane
Copy link
Member

Thanks so much for the feedback, @yarikoptic !

It is a lot of text -- we're hoping that by being very explicit about these ideas in the beginning, it will be easier for new contributors to hit the ground running 🏃

Just building on this point from @emdupre - I think the goal is not to expect everyone to read all the text the first time they come to the proejct, but to have it there so the maintainers are consistant in how they answer questions to bring more people into the project.

Also added an example of combining labels.
Totally fine to reject this change if you think its too verbose :)
Also added an example of combining labels.
Also standardised how they're referred to
between the different milesones :)
@KirstieJane
Copy link
Member

I helped to write a lot of this so it isn't terribly surprising that I like it A LOT. But I do. I think it's super helpful to be so clear about where we're going and what we're trying to do!

I've added a few comments in a PR to @emdupre's branch: emdupre#18.

Thank you all for the awesome hard work!

@emdupre emdupre mentioned this pull request Nov 5, 2018
@rmarkello
Copy link
Member

I'm sorry I've been delinquent in commenting on this, but I am a huge fan of all of these changes 🙌

I love the rhythm of the roadmap, and the updated contributing is great. I think even if it's not something a first-time user would necessarily go through in depth, it definitely showcases the thought being put into it all!

My one question / clarification is with regards to the Governance section in the contributing.rst file: I feel it's currently a bit vague in terms of what the governance structure actually is. In the CODE_OF_CONDUCT.md @emdupre is referenced as the "interim BDFL", and there's reference to the small group of core contributors within the contributing.rst, but I was wondering whether it might be worthwhile discussing e.g., the decision-making process? I know it might be a bit premature to have strict guidelines, but currently my reading of that section is "We have a governance structure and are committed to it!" without much description of the structure itself.

Perhaps a good interim position here would be something like:

As BDFL, @emdupre is ultimately responsible for any major decisions pertaining to tedana development. However, all potential changes are explicitly and openly discussed in the described channels of communication and we strive for consensus amongst all community members.

@emdupre
Copy link
Member Author

emdupre commented Nov 8, 2018

Thanks so much for the feedback, @rmarkello ! Your point about not (yet) being explicit with an actual governance structure is a good one, but I am worried that we're much too small to have a more formal one like Rust or NumPy.

I do, though, like specifying an interim position as you suggested ! I've incorporated it into this PR, but if anyone in @ME-ICA/tedana-devs has other opinions please let me know !

KirstieJane and others added 3 commits November 9, 2018 13:10
)

* Add text from HBClab/NiBetaSeries to contributing guidlines

Explains a little about markdown and a
little about rstructured text :)

This text is taken from jdkent's contribution to the NiBetaSeries
project, which built on my contribution to
the BIDS Starter Kit!
I'll try to attribute him in the pull request so he gets some
credit 😺

* Update CONTRIBUTING.md

Co-authored-by: james-kent@uiowa.edu

(Actually it was the commit before - I don't know how to fix iiiiiit! I just want him to show up on the contributors list!)

* Add guide to markdown and rst

Co-authored-by: jdkent <james-kent@uiowa.edu>

Thank you to James for the text from
the HBClab/NiBetaSeries project (PR ME-ICA#77)

* put table of contents in the right order
We are committed to providing helpful documentation for all users of ``tedana``.
One metric of success, then, is to develop documentation that includes:

1. Motivations for conducting echo time dependent analysis,
Copy link
Member Author

Choose a reason for hiding this comment

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

Feedback from Javier suggest that we should also include guidance on collecting ME data, or at least pointers to appropriate resources.

**Summary**:
Overall, each of the listed deliverables will support a broader goal:
to improve on ME-EPI processing itself.
This is an important research question and will advance the state-of-the-art in ME-EPI processing.
Copy link
Member Author

Choose a reason for hiding this comment

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

Javier suggests that measuring quality assurance here will be an important part of this process. I agree, as noted in #153 (comment)

@emdupre
Copy link
Member Author

emdupre commented Nov 13, 2018

Ok, we're merging this today ! 🎉 ✨

Thanks so much to everyone for the feedback. These are all documents we can (and should !) be revisiting as development continues, so please (always) feel free to open an issue with any suggestions moving forward !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community issues related to building a healthy community documentation issues related to improving documentation for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants