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

[INFRA] updating remark, CIs, contributor docs #745

Merged
merged 21 commits into from
Mar 8, 2021

Conversation

sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented Mar 5, 2021

was also trying to get a global install to work, but it seems tricky.

Perhaps it'd be easier to do a local install both here in the CI, and as an advertisement in the CONTRIBUTING guide.

We'd then add node_modules, package.json and package-lock.json to our .gitignore file so that nobody accidentally commits these folders/files, because they are created during local npm installs.

See also: https://github.com/remarkjs/remark-lint

@sappelhoff sappelhoff changed the title updating remark [INFRA] updating remark, CIs, contributor docs Mar 5, 2021
@sappelhoff sappelhoff requested a review from Remi-Gau March 5, 2021 15:33
@sappelhoff sappelhoff added this to the 1.6.0 milestone Mar 5, 2021
@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Mar 5, 2021

does runnning remark still mess us the macro code mentioned in that other PR (see comment ).

Or should I:

  • A) open an issue about this (and take care of it in another PR)
  • B) see if we can get the linter to ignore those sections?

@sappelhoff
Copy link
Member Author

does runnning remark still mess us the macro code mentioned in that other PR (see comment ).

I didn't check -- but I think that's still an issue.

I would go for A) --> opening an issue and doing a PR next time we have time and energy for it

but you can also push to this PR if you'd rather do B)

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Mar 7, 2021

does runnning remark still mess us the macro code mentioned in that other PR (see comment ).

I didn't check -- but I think that's still an issue.

I would go for A) --> opening an issue and doing a PR next time we have time and energy for it

but you can also push to this PR if you'd rather do B)

I quickly check locally and adding :

<!-- lint disable -->

Does not seem to prevent remark to try to fix those "macros": so I won't look further into it for now, but I will add a message in the CONTRIBUTING.md to tell contributors to be careful which hunk they commit after fixing.

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Mar 7, 2021

Strangely enough even though remark does not flag files as problematic it still fixes stuff when asked to.

The most annoying (potentially problematic)

  • changing unordered list from using - to using *
  • changing ordered list from using 1. to actually using the number of the item
  • changes literal hyperlinks URLs from [URL](URL) to <URL>

Clearly not the end of the world but someone using this fix could end up committing quite a few extra things

Could look into a finer control of remark But I would flag that as low priority for the reason that we have not seen that happening all over the place yet.


More problematic the tendency to escape all _ and & with a \ in all the URLs.


In general, from a new user perspective, all these extra changes could really look like they broke everything and could be very confusing.

Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

LGTM

@sappelhoff sappelhoff merged commit 1d7b363 into bids-standard:master Mar 8, 2021
@sappelhoff sappelhoff deleted the remark branch March 8, 2021 07:50
@sappelhoff
Copy link
Member Author

Thanks for review and suggestions Remi!

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.

2 participants