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

A few updates to the roadmap and contributing guidelines #18

Merged
merged 14 commits into from
Nov 6, 2018

Conversation

KirstieJane
Copy link

@KirstieJane KirstieJane commented Nov 3, 2018

In reference to pull request ME-ICA#151

Changes proposed in this pull request:

  • Add opinionated description of whether tedana is backwards compatible with MEICA
  • Add links to milestones and standardise how they're referred to ("Associated milestone" vs "Milestone")
  • Change (again) a tiny phrasing as relates to the code of conduct
  • Add example of multiple tags for pull requests
  • Add a couple more guiding comments to the pull request template - feel free to ignore if they're too verbose
  • Add vscode to gitignore file

What became VERY apparent is that I don't know how to write .rst and I haven't built the docs to check, so I think my edits will need a look over for formatting. SORRY 😭

Also, I made a mess of the different commits in my git history and I haven't cleaned them up because I don't want to make more of a mess! But if you'd like me to do a better job there I can do 😬

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 KirstieJane changed the base branch from master to doc/roadmap November 3, 2018 21:52
Copy link
Owner

@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.

Thanks so much, @KirstieJane ! Your rst formatting is perfect, actually 🎉

I did have a few small requests, but most of them are stylistic. Thanks so much for these edits and your always extremely helpful feedback !!!

@@ -9,8 +9,12 @@ See here for more information and a list of available options:
http://tedana.readthedocs.io/en/latest/contributing.html#pull-requests
Copy link
Owner

Choose a reason for hiding this comment

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

Could you update this URL to point to the new CONTRIBUTING.md section on pull requests ? Otherwise this will return a 404 ! Sorry for missing it myself !!

Copy link
Author

Choose a reason for hiding this comment

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

Done ✅

@@ -132,6 +132,29 @@ mean that this work should be relatively manageable.
We hope that the lessons we learn building something useful in the short term will be
applicable in the future as other needs arise.

.. _backwards compatibility with meica:
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add this section to the TOC ? Also, I think it might make sense to place this before "how does tedana future-proof its development" since that directly feeds into the release checklist !

Copy link
Author

Choose a reason for hiding this comment

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

Done ✅


The short answer is no.

There are two main reasons why. One is that the tool originally used to run the independent
Copy link
Owner

Choose a reason for hiding this comment

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

Can we put each sentence on its own line ?

Copy link
Author

Choose a reason for hiding this comment

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

Yep! 🙌

The other reason is a choice of the core developers to look forwards rather than maintaining
an older codebase. As is described `above`_, ``tedana`` is developed by a small team of
volunteers and they have to allocate their time accordingly. If you'd like to use MEICA as has
been previously published the code is available on `github`_ and freely available under a LGPL2
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should actually link to the bitbucket here, rather than the github, since the version on github has actually not previously been published on !

Copy link
Author

Choose a reason for hiding this comment

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

So, the bitbucket link says to go to the github repo....and I figured if someone is trying to use MEICA it would be super annoying to have to go to bitbucket just to then be sent to the github link......

But I know what you mean, so happy to change if you'd like?

Copy link
Author

Choose a reason for hiding this comment

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

This is the only part of your review I haven't done @emdupre - let me know what you think. This is one of my only moderately held opinions....

Copy link
Owner

Choose a reason for hiding this comment

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

I see your point here, for sure -- really this is a problem in the bitbucket. We can add it to our to-do list once ME-ICA#119 is merged and the two do really differ

Copy link
Author

Choose a reason for hiding this comment

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

Coolio! Updated - I'll push that change now ✨

exactly reproducing MEICA analyses is not possible.

The other reason is a choice of the core developers to look forwards rather than maintaining
an older codebase. As is described `above`_, ``tedana`` is developed by a small team of
Copy link
Owner

Choose a reason for hiding this comment

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

We could actually link this directly out with

As described in the ``tedana`` :ref:`governance` description,

I'm actually not sure where above_ is linking to right now ?

Copy link
Author

@KirstieJane KirstieJane Nov 5, 2018

Choose a reason for hiding this comment

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

Lolz. Yep. I think I did at some point have a link for "above" but it isn't there anymore!

I've changed this to:

As described in the :ref:`governance` section, ``tedana`` is developed by a small...

Is that the right formatting??

Copy link
Owner

Choose a reason for hiding this comment

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

It is ! but sorry, we need to add the link in-text. So above at L11 it should be:

.. _governance:

Governance
----------------

Copy link
Author

Choose a reason for hiding this comment

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

DONE 👾

docs/roadmap.rst Outdated
@@ -51,11 +51,13 @@ One metric of success, then, is to develop documentation that includes:

.. _a ReadTheDocs site: https://tedana.readthedocs.io

**Associated Milestone**:
**Associated Milestone**: `#6`_
Copy link
Owner

Choose a reason for hiding this comment

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

Great call ! But the links aren't going to play nicely with the pound signs. Could we actually just wrap it s.t. the links each look like

**`Associated Milestone`_**

.. _Associated Milestone: https://github.com/ME-ICA/tedana/milestone/6

?

Copy link
Author

Choose a reason for hiding this comment

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

Yep - see - I knew I didn't know the rst format well enough! I'll make these changes now 😸

@@ -9,8 +9,12 @@ See here for more information and a list of available options:
http://tedana.readthedocs.io/en/latest/contributing.html#pull-requests
-->

<!-- Please indicate after the # which issue you're closing with this PR -->
Copy link
Owner

Choose a reason for hiding this comment

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

😍 This is great ! Should we explain that this will automatically close the issue, maybe by including this link ?

@@ -119,6 +119,10 @@ If your pull request is not yet ready to be merged, please also include the **[W
This tells the development team that your pull request is a "work-in-progress",
and that you plan to continue working on it.

You can also combine the tags above, for example if you are updating both a test and
Copy link
Owner

Choose a reason for hiding this comment

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

Same question here about new lines !

Copy link
Author

Choose a reason for hiding this comment

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

Done 👾

@KirstieJane
Copy link
Author

Thanks for the feedback @emdupre! ✨ 💖 💪

@emdupre
Copy link
Owner

emdupre commented Nov 6, 2018

Hooray !!! Thanks for the edits. I had one suggestion to fix a link but this is otherwise ready ! If you don't have time to update the PR (since I know you're at a conference 🌯🌞) I can merge and then patch the link myself :)

@emdupre
Copy link
Owner

emdupre commented Nov 6, 2018

Thanks !! 🎉✨ Merging now !

@emdupre emdupre merged commit a1a8f5e into emdupre:doc/roadmap Nov 6, 2018
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.

2 participants