[MISC] standardize string examples format in tables

[Remi-Gau]

This PR fixes #715

closes #686

• update contributing.md

  • updates information to install mkdocs and plug-ins
  • mention "soft rules" regarding formating
    • "start sentence on new line"
    • hard word wrap to limit line length
    • explain format for json examples in tables

• uniformize json string examples in tables in the specs

I have not touched the task_events.md as it "under reconstruction" in PR #697

I will take care of the table linting once we are OK on the "content".

[Remi-Gau]

2 other issues I have noticed that make my inner OCD unhappy:

• inconsistency in the format of the table headers in the spec: most of the time it is in bold but not always
• numeric examples in the tables concerning the content of json files are not "highlighted" and might not be as obvious to the reader.

For example

Number of EEG channels included in the recording (for example, 128).

Should probably be:

Number of EEG channels included in the recording (for example, `128`).

• inconsistencies in the rendering of string and numeric values for tables (mostly for stings)

Sometimes examples are written:

like this

and other times

`like that`

Column name	Requirement level	Description
name	REQUIRED	Channel name (for example, FC1, Cz)
units	REQUIRED	Physical unit of the value represented in this channel, for example, V for Volt, or fT/cm for femto Tesla per centimeter (see Units).

If there is no objection I will open a separate issue and tackle this after this PR is solved (reviewing those formating issues will be easier if they are done one at a time).

[sappelhoff]

If there is no objection I will open a separate issue and tackle this after this PR is solved (reviewing those formating issues will be easier if they are done one at a time).

sounds good to me, let's get this one in first 👍

[sappelhoff]

Cool, thanks for this contribution! :)

I left a few comments, but I am eager to merge this once all are resolved and at least one more person reviewed.

[sappelhoff]

ready to be merged from my side - once tables pipes are fixed 👍

[Remi-Gau]

ready to be merged from my side - once tables pipes are fixed +1

OK that might actually trigger another PR because last time I tried to follow our own instruction on how to use remark I was getting a bunch of errors.

Need to check that it is not a case of ERROR 101: incompetent user on my part. 😉

[tsalo]

Thanks for this! I noticed some minor typos in the contributing guidelines, so I've added suggestions to fix them. A few of them are more about my wording preferences, so feel free to take or leave those ones.

[Remi-Gau]

Thanks for this! I noticed some minor typos in the contributing guidelines, so I've added suggestions to fix them. A few of them are more about my wording preferences, so feel free to take or leave those ones.

Wow! Thanks!

I had peppered those around, hadn't I?

[Remi-Gau]

And of course there is merge conflict... Sigh...

[Remi-Gau]

OK I am trying to follow our own instructions in the contributing guidelines and it seems remark is failing me.

Tried with several versions of Node with nvm and... nope.

npm install `cat npm-requirements.txt`   # runs fine
cd src/04-modality-specific-files/
remark 01-magnetic-resonance-imaging-data.md -o 01-magnetic-resonance-imaging-data-fixed.md 
remark: command not found          # OOOPS

I suspect I am messing up something in the install, but not sure what.

[sappelhoff]

I remember having issues with that as well when I was setting up the CI flow:

bids-specification/.github/workflows/markdown_style.yml

Lines 1 to 18 in 9cb5612

	name: Check Markdown style
	on: [push, pull_request]
	jobs:
	markdown-style:
	runs-on : ubuntu-latest
	steps:
	- uses: actions/checkout@v2
	- name: Setup NodeJS
	uses: actions/setup-node@v1
	with:
	node-version: 10
	- name: Install dependencies
	run: npm install `cat npm-requirements.txt`
	- name: Run style checks
	# try npx: https://stackoverflow.com/a/45164863/5201771
	run: npx remark src/*.md src/*/*.md --frail

however back then I thought the complications were due to the CI environment and at some point I just wanted it to work and then dropped the issue as soon as it did ... :-/

[sappelhoff]

Maybe this is helpful?

bids-specification/.circleci/config.yml

Lines 89 to 131 in 9cb5612

	# Clear remark test
	remark:
	working_directory: ~
	docker:
	- image: node:latest
	steps:
	- checkout
	- attach_workspace:
	at: ~/build
	- run:
	name: update-npm
	command: |
	cd ~
	npm install npm@latest
	- run:
	name: get remark
	command: |
	cd ~
	npm install remark remark-cli
	- run:
	name: get remark styles
	command: |
	cd ~
	cat ~/project/npm-requirements.txt | xargs npm install
	- run: # remark the auto generated changes.md
	name: remark on autogenerated CHANGES.md
	command: |
	cd ~/project
	if (git log -1 --pretty=%s | grep Merge*) && (! git log -1 --pretty=%b | grep REL:) ; then
	mkdir ~/project/src/tmp
	cat ~/build/src/CHANGES.md
	cp ~/build/src/CHANGES.md ~/project/src/CHANGES.md
	~/node_modules/.bin/remark ~/project/src/CHANGES.md -o ~/project/src/tmp/CHANGES.md
	~/node_modules/.bin/remark ~/project/src/tmp/CHANGES.md --frail
	else
	echo "Commit or Release, do nothing"
	mkdir ~/project/src/tmp
	touch ~/project/src/tmp/empty.txt
	fi
	- persist_to_workspace:
	root: ~/project/src
	paths: tmp

[Remi-Gau]

ah yeah that worked so you use "local" install of remark to run this...

So:

1. our doc does not say that.
2. I am no Node expert, but that feels "weird" to call remark that way when other places on the interweb say you can just run remark.

I suggest:

• lint the remaining files that way in this PR and merge
• fix our contributing doc by either saying to run that command or find the right install instruction for this.

[sappelhoff]

Yes, so in https://github.com/bids-standard/bids-specification/blob/master/CONTRIBUTING.md#fixing-travis-remark-errors we instruct users to do a "local" install of remark, which produces several files as well:

also: We are not using travis CI anymore, but have switched to GH-actions

Finally, I am not sure anymore why we run remark in BOTH, the gh-actions job, AND in circleci.

To conclude: there is enough of a mess to warrant a new PR, I think :-D

EDIT: One more, the packages we use for linting are outdated, and one of them raises a security warning 🤣

[Remi-Gau]

FYI

remark seems to get over zealous and changes @tsalo pretty pretty macro code:

From this

{{ MACROS___make_filename_template(datatypes=["anat"]) }}

into this

{{ MACROS\_\_\_make_filename_template(datatypes=["anat"]) }}

Will add this to the list of things to mention in the doc and possibly see if we can tweak some things to be ignored by the linter (I think I did this in the past).

[Remi-Gau]

OK another one to throw in the bucket: running remark locally to fix things adds extra changes even the file has no issue.

Example of the lines added below.

I am thinking of "fixing" those as well (not in this PR):

• when running remark it means you have to ignore those chunks when committing
• if a change is made voluntarily in those sections it could be missed or make the git diff more confusing

<!-- Link Definitions -->

[object]: https://www.json.org/json-en.html

[objects]: https://www.json.org/json-en.html

[number]: https://www.w3schools.com/js/js_json_datatypes.asp

[integer]: https://www.w3schools.com/js/js_json_datatypes.asp

[string]: https://www.w3schools.com/js/js_json_datatypes.asp

[arrays]: https://www.w3schools.com/js/js_json_arrays.asp

[uri]: ../02-common-principles.md#uniform-resource-indicator

[Remi-Gau]

@tsalo let me know if you still have things you'd like me to fix on this.

😺

[tsalo]

LGTM!

[sappelhoff]

Thanks @Remi-Gau and @tsalo for reviewing