-
Notifications
You must be signed in to change notification settings - Fork 95
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
Conversation
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 Report
@@ 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.
|
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 ? |
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 ! |
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.
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 |
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.
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
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.
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.
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.
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.
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. |
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.
Silent failures... Not sure what those are. May be just say bugs? Could be clarified (leading to incorrect computation, failing execution, etc)
docs/contributing.rst
Outdated
|
||
Pull Requests | ||
````````````` | ||
As is stated in the code, severe or repeated violations by community members may result in exclusion |
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.
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 |
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 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:
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 |
I like the updated contributing guidelines as well. 👍 overall for the PR. |
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 :)
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! |
Also fix a couple of typos & make sure all new sentences start on new lines
Put new sentence on a new line 😃
A few updates to the roadmap and contributing guidelines
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 Perhaps a good interim position here would be something like:
|
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 ! |
) * 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: [email protected] (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 <[email protected]> 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, |
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.
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. |
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.
Javier suggests that measuring quality assurance here will be an important part of this process. I agree, as noted in #153 (comment)
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 ! |
Closes #131, #148.
Changes proposed in this pull request:
tedana
, added to the RTD siteThis 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 !