[DOC] Generalize installation instructions to work with Windows in CONTRIBUTING #846
Conversation
Codecov Report
@@ Coverage Diff @@
## main #846 +/- ##
=======================================
Coverage 93.27% 93.27%
=======================================
Files 27 27
Lines 2217 2217
=======================================
Hits 2068 2068
Misses 149 149 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you for contributing! There are just a few things that will need to be changed before I can approve.
Can you change the PR title and summary comment to follow tedana's conventions?
The PR title should have a prefix in brackets (see the list here) and a descriptive name. For example, something like "Generalize installation instructions in CONTRIBUTING" would work.
Also, the summary comment should link to the issue using GitHub's keywords (e.g., Closes #845) so that the issue will automatically be closed when the PR is merged. The link as you have it now won't do that.
EDIT: I would also recommend linking to #844 as well. You will need to use the same keyword (closes, addresses, etc.) before each issue link. Something like Closes #845 and #844 won't work.
CONTRIBUTING.md
Outdated
|
|
||
| If the above command didnot worked for you try using the following command : | ||
| ``` | ||
| pip install -e .[all] | ||
| ``` |
There was a problem hiding this comment.
Rather than adding more steps to try, I think it would be best to make our standard installation instructions work for as many people as possible. Why not change the instructions above to pip install -e .[all]?
There was a problem hiding this comment.
That does not work on every shell unfortunately; in my zsh (MacOS default) it attempts to expand the shell and then there are no valid expansions, so it exits with error:
zsh: no matches found: .[all]
so we do indeed need quotes for UNIX systems and non-quotes for Windows.
There was a problem hiding this comment.
That's unfortunate, but I'm glad to know definitively that that's the case.
There was a problem hiding this comment.
However, it looks like pip install -e . works on both Windows and UNIX systems, so we should just change the instructions to that.
There was a problem hiding this comment.
But it does omit dev dependencies. I pushed some changes to simplify the language a bit, confirmed working on the relevant platforms.
handwerkerd
changed the title
|
Thank you @aryangupta701 for identifying this info and figuring out a solution! I'm very sorry for my delayed response. It's been a busy few weeks and I'm trying to catch up on standing issues before our tedana developers' call this Thursday. I just made changes to the PR title and your opening comment to conform to our conventions in response to @tsalo's comments. If you submit other PRs, I'd be happy to help guide you through conventions that might be specific to this project. I do agree with Taylor that, if Also, our dev call is this Thursday at this local time. The issue with the zoom link is #857 If you want to learn more about tedana, you are welcome to join. |
|
@aryangupta701 I hope you don't mind but I've pushed some changes to your branch (this way you get co-author credit and PR credit) so that we can merge this change with a few updates. |
Closes #844, #845
Changes proposed in this pull request:
pip install -e .'[all]'does not work on Windows whilepip install -e .[all]does. This changes CONTRIBUTING.md to recommend this step without the added quotes.