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: Swap Markdownlint for remark-lint #2580

Merged

Conversation

nschonni
Copy link
Member

Re-opening #2460 because GitHub closed it on a force-push

@nschonni nschonni force-pushed the chore--Swap-Markdownlint-for-remark-lint branch from 71e5f8d to 4a44f6a Compare September 16, 2019 01:07
@nschonni nschonni force-pushed the chore--Swap-Markdownlint-for-remark-lint branch 4 times, most recently from da9d483 to c44b515 Compare October 1, 2019 04:01
@XhmikosR
Copy link
Contributor

XhmikosR commented Oct 1, 2019

I'm definitely in favor of this assuming nodejs/remark-preset-lint-node#21 lands, released and this PR makes the minimum possible overwrites.

The more closer we are to the upstream rules there better. My main concern is the different markdown renderer we use here, but hopefully it won't cause any issues.

@nschonni nschonni force-pushed the chore--Swap-Markdownlint-for-remark-lint branch 4 times, most recently from 403fc5a to 5d51daf Compare October 12, 2019 03:27
@XhmikosR
Copy link
Contributor

@nschonni are the current rules the minimum needed ones? Do we cover most of what we covered before?

Also, I wonder, does remark-lint have a cache feature?

@nschonni
Copy link
Member Author

I don't think this actually covers everything that Markdownlint does, but it has some more that it doesn't. A PR landed to added the "recommended" preset to the node baseline, but it needs a new release to include it.
Not sure what you mean by caching in this case

@Trott
Copy link
Member

Trott commented Oct 14, 2019

A PR landed to added the "recommended" preset to the node baseline, but it needs a new release to include it.

Would it be useful for me to publish a release at this time?

@nschonni
Copy link
Member Author

Sure, maybe land the other minor dependency cleanup one first though.

@nschonni nschonni force-pushed the chore--Swap-Markdownlint-for-remark-lint branch from 63d5c41 to f01a3a1 Compare October 14, 2019 04:51
@nschonni
Copy link
Member Author

For now, I've submitted a few PRs so some of the rules can be re-enabled

@Trott
Copy link
Member

Trott commented Oct 14, 2019

Sure, maybe land the other minor dependency cleanup one first though.

Published.

@nschonni nschonni force-pushed the chore--Swap-Markdownlint-for-remark-lint branch from f01a3a1 to fe4f9c4 Compare October 14, 2019 05:52
@nschonni
Copy link
Member Author

Thanks, bumped the package and disabled newly failing rules

@XhmikosR
Copy link
Contributor

Re: caching, I meant like stylelint or eslint has, so that consequent runs are faster. Maybe it doesn't have this feature, no big deal for now.

So, to summarize:

  1. I personally care about covering most of the things we covered before
  2. We need to get the suppressions as close to zero eventually

That being said, this shouldn't block us from merging this. We can tackle the rules as we go.

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

RSLGTM

@nschonni nschonni force-pushed the chore--Swap-Markdownlint-for-remark-lint branch 3 times, most recently from e855e52 to f6e8cd9 Compare October 16, 2019 21:54
package.json Outdated
"nock": "^11.4.0",
"pre-commit": "^1.2.2",
"proxyquire": "^2.1.3",
"remark-cli": "^7.0.0",
"remark-frontmatter": "^1.3.2",
"remark-preset-lint-node": "^1.10.1-0",
Copy link
Member

Choose a reason for hiding this comment

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

Are we good to land this once we bump to 1.10.1?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, I've re-installed and rebased

The main node repo has styleguide defined in remark-preset-lint-node
@nschonni nschonni force-pushed the chore--Swap-Markdownlint-for-remark-lint branch from f6e8cd9 to 71637a4 Compare October 18, 2019 02:30
@Trott Trott merged commit 7b1db68 into nodejs:master Oct 18, 2019
@nschonni nschonni deleted the chore--Swap-Markdownlint-for-remark-lint branch October 18, 2019 03:05
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.

5 participants