Skip to content

[MISC] Fixing Travis errors with Remark#320

Merged
effigies merged 10 commits intobids-standard:masterfrom
franklin-feingold:contrib_remark
Sep 18, 2019
Merged

[MISC] Fixing Travis errors with Remark#320
effigies merged 10 commits intobids-standard:masterfrom
franklin-feingold:contrib_remark

Conversation

@franklin-feingold
Copy link
Copy Markdown
Collaborator

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

sappelhoff
sappelhoff previously approved these changes Sep 4, 2019
Copy link
Copy Markdown
Member

@sappelhoff sappelhoff left a 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

Copy link
Copy Markdown
Collaborator

@effigies effigies left a 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`.
Copy link
Copy Markdown
Collaborator

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.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this a worry?

Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
Collaborator

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>
Copy link
Copy Markdown
Collaborator

@effigies effigies left a 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:

- 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)
```
Copy link
Copy Markdown
Collaborator

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)

Copy link
Copy Markdown
Collaborator Author

@franklin-feingold franklin-feingold Sep 13, 2019

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

Copy link
Copy Markdown
Collaborator

@effigies effigies left a 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

Copy link
Copy Markdown
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Thanks Franklin!

@effigies effigies merged commit c446f5e into bids-standard:master Sep 18, 2019
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.

3 participants