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

(chore) Minor Markdown tweaks #1256

Merged
merged 1 commit into from
Dec 27, 2019
Merged

(chore) Minor Markdown tweaks #1256

merged 1 commit into from
Dec 27, 2019

Conversation

XhmikosR
Copy link
Contributor

No description provided.

Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

👋 rather than manually tweaking the markdown layout, could we pull in prettier and add markdown to what we lint?

@XhmikosR
Copy link
Contributor Author

I just ran this through markdownlint. Your call about adding prettier, although it should definitely be more consistent.

Which reminds me, I missed the docs. I will update the PR.

@bcoe
Copy link
Member

bcoe commented Dec 27, 2019

@XhmikosR markdownlint would totally be fine too 👍 I'd just be tempted to try to automate this process, as well as checking in the fixes; make npm run lint fail if markdownlint fails.

@coreyfarrell
Copy link
Member

I am not a fan of prettier for JS code. The demands it makes about line length are very annoying and IMO actually make the code more difficult to read.

I don't know what prettier would do the markdown, I don't have a strong opinion about using prettier for things that are not JS.

@bcoe
Copy link
Member

bcoe commented Dec 27, 2019

I don't have a strong opinion about the linter we use, but if we want to enforce a style on folks, I like it to be automated ... otherwise contributors end up getting feedback on their edits to README.md based on conventions floating around in someone's head.

@XhmikosR
Copy link
Contributor Author

TBH I don't like prettier either. Not sure how it performs on Markdown files.

An alternative would be to use remark-lint or markdownlint. Again, I'm not sure how good their autofix works, which is the strong point of prettier.

@coreyfarrell
Copy link
Member

I'm in favor of adding markdownlint as a dev dependency and running it from npm run lint.

@XhmikosR
Copy link
Contributor Author

I could make a PR with markdownlint-cli, just keep in mind its auto-fixing it's not like prettier where everything is fixed for the user (IIRC).

@XhmikosR
Copy link
Contributor Author

Just so we are safe, are the doc Markdown files used somewhere else apart from GitHub?

@coreyfarrell
Copy link
Member

README.md of the latest release is displayed at https://npmjs.com/package/nyc. Other than that I think it's github only.

Oh before I forget CHANGELOG.md would need to be excluded by any automated lint, it is automatically generated by standard-version.

@XhmikosR
Copy link
Contributor Author

OK, so we should be fine. I think this PR can be merged and I'll try to make a PR for markdownlint later.

@coreyfarrell coreyfarrell merged commit bed4729 into istanbuljs:master Dec 27, 2019
@coreyfarrell
Copy link
Member

I talked with @bcoe and we feel that using prettier would be OK as long as it were configured to ignore JS and TS code. I'm actually leaning towards using xo for the actual linter but if you don't mind please allow me to push that change as I'll likely want to disable a couple rules and it's easier for me to make those decisions if I work through the switch myself.

@XhmikosR XhmikosR deleted the patch-1 branch December 28, 2019 12:02
@XhmikosR
Copy link
Contributor Author

Your project, your call :)

I actually did use xo in #1253 (comment) but with a config pretty opinionated so that I ignored the style changes

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