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

[FIX] number of small corrections to the specification #98

Merged
merged 17 commits into from
Feb 12, 2019

Conversation

robertoostenveld
Copy link
Collaborator

These are mostly formatting changes and fixes to external links.

Copy link
Collaborator

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

This ain't a complete review, but a few notes how to make PR better ;)
THANK YOU btw -- we should indeed polish up the spec so there is no place for ambiguity and errors like the ones you (and now me ;-) ) point out!

src/02-common-principles.md Outdated Show resolved Hide resolved
src/02-common-principles.md Show resolved Hide resolved
@chrisgorgo
Copy link
Contributor

Thanks for sending this along, FYI - here are the linter errors Travis spotted: https://travis-ci.com/bids-standard/bids-specification/builds/92812807

@yarikoptic
Copy link
Collaborator

FWIW +1 on formalizing to use American English, and mention that in common principles of it isn't there yet

@chrisgorgo
Copy link
Contributor

chrisgorgo commented Nov 29, 2018 via email

yarikoptic
yarikoptic previously approved these changes Dec 5, 2018
@yarikoptic
Copy link
Collaborator

@robertoostenveld would you have time to resolve outstanding issues causing CIs to fail? I think we are ready otherwise and should merge!

@yarikoptic
Copy link
Collaborator

@robertoostenveld - I have pushed a commit resolving introduced problems. If would be all kosher - I would merge or we would be in jeopardy to introduce conflicts

@robertoostenveld
Copy link
Collaborator Author

The Christmas period delayed work on this. I have updated to the HEAD of the master branch and resolved the conflicts. I see that circleci passes, but that travis fails: I will investigate.

@robertoostenveld
Copy link
Collaborator Author

Whereas travis initially was showing "Incorrect list-item indent: add 2 spaces", after me changing it it to 2 spaces it was showing "Incorrect list-item indent: add 1 space". Also the table fence around the emojis seems impossible to get right. I am lost!

Content wise it all seems fine to me, so I suggest to merge.

@chrisgorgo
Copy link
Contributor

Consider using https://github.com/prettier/prettier to automatically fix syntax

@robertoostenveld
Copy link
Collaborator Author

... so I managed to get all checks to pass!

@chrisfilo: the opinion of prettier regarding the table fence turns out not consistent with that implemented using remark-lint in Travis. I guess it is because the different emoji having different byte counts, and the tools not agreeing on how this is to be dealt with.

Furthermore, remark-lint gives me Error: Could not find module lint-list-item-indent, both on the command line and in Atom. So linting - although a nice idea - does not work for me (as a non computer scientist). I wonder how other neuroscientists are dealing with this, especially the ones for whom WordPad is the default tool for the editing of text files.

@chrisgorgo
Copy link
Contributor

I agree, but this is a good topic for a new issue (perhaps proposing less strict linting).

@chrisgorgo
Copy link
Contributor

@yarikoptic @robertoostenveld is this read to merge?

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

Looks good to me ... except for in the contributors file, where a line seems to be removed: If you can tell me why (or fix this "typo"), I can approve and merge this.

src/99-appendices/01-contributors.md Outdated Show resolved Hide resolved
Added missing contributor back
@robertoostenveld
Copy link
Collaborator Author

this is ready to merge.

@sappelhoff sappelhoff changed the title number of small corrections to the specification [MRG] number of small corrections to the specification Feb 11, 2019
Copy link
Collaborator

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@yarikoptic yarikoptic merged commit e61a4dc into bids-standard:master Feb 12, 2019
@chrisgorgo
Copy link
Contributor

@yarikoptic thanks.

This is minor, but please wait 5 days since the last commit before merging next time (as per https://github.com/bids-standard/bids-specification/blob/master/DECISION-MAKING.md#rules). At least until someone automates this (see #147).

@yarikoptic
Copy link
Collaborator

Ok, noted

@sappelhoff sappelhoff changed the title [MRG] number of small corrections to the specification [FIX] number of small corrections to the specification Sep 8, 2020
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.

4 participants