-
Notifications
You must be signed in to change notification settings - Fork 159
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
[MISC] Fixing Travis errors with Remark #320
[MISC] Fixing Travis errors with Remark #320
Conversation
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.
very sensible to include this, thanks @franklin-feingold
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.
Overall looks good. Some suggestions.
CONTRIBUTING.md
Outdated
``` | ||
|
||
This command will return whether this file can pass Travis. | ||
If it passes, the `flagged_file_fixed.md` can update the `flagged_file.md`. |
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.
Suggesting a code block doesn't work well, so just using straight markdown below:
If it passes, replace flagged_file.md
with the contents of flagged_file_fixed.md
,
add
and commit
the change:
mv flagged_file_fixed.md flagged_file.md
git add flagged_file.md
git commit -m 'STY: Fixed Markdown style'
CONTRIBUTING.md
Outdated
|
||
This command will return whether this file can pass Travis. | ||
If it passes, the `flagged_file_fixed.md` can update the `flagged_file.md`. | ||
Please ensure the names are the same before pushing the fixed file to your forked repository. |
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.
Is this a worry?
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.
shouldn't the files be named the same so it gets updated properly? we don't want the bad and fixed next to each other
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.
Right, it just seems a bit obvious. Whatever, though.
Co-Authored-By: Chris Markiewicz <effigies@gmail.com>
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 think I was unclear with shell terminology, so I've suggested an updated section 2. Can you also update .travis.yml
:
bids-specification/.travis.yml
Line 8 in 3380de8
- npm install remark-cli@5.0.0 remark-lint@6.0.2 remark-preset-lint-recommended@3.0.2 remark-preset-lint-markdown-style-guide@2.1.2 |
CONTRIBUTING.md
Outdated
|
||
``` | ||
npm install $(cat npm-requirements.txt) | ||
``` |
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.
Oh man, I wrote the comment and forgot to hit "Start a review", so I lost it. I checked, and we can use back-ticks on Bourne-compatible shells (i.e., sh, bash, dash, zsh) as well as csh and ksh, which makes up probably 99.9% of Unix shells.
Here's the suggested update to section 2:
2. Install Remark-CLI and our style guide
Remark-CLI can be installed via npm, which is part of
the NodeJS distribution.
To install the packages we use for our style guide, the following command will
work on most command lines:
npm install `cat npm-requirements.txt`
The equivalent command on PowerShell is:
npm install @(cat npm-requirements.txt)
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.
updated with your suggestion and updated travis. Thank you
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.
Thanks for your patience
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.
Thanks Franklin!
Addresses a common error we see with Travis flagging files because of style issues. Added a short tutorial of how to use Remark to fix this error