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: do not lint markdown ignored files #16057

Closed

Conversation

amareshsm
Copy link
Member

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
Do not lint markdownlint ignored files.

What changes did you make? (Give an overview)

Before:
node_modules markdown files are getting linted when we run the npm run lint command.

Screenshot 2022-06-19 at 5 55 01 PM

After changes:

Ignored all .markdownlintignore files.

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon bug ESLint is working incorrectly labels Jun 25, 2022
@netlify
Copy link

netlify bot commented Jun 25, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit b0b8383
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/62b771dfae1b98000902c566

@amareshsm amareshsm marked this pull request as draft June 25, 2022 20:27
@@ -71,8 +71,7 @@ const NODE = "node ", // intentional extra space
// Files
RULE_FILES = glob.sync("lib/rules/*.js").filter(filePath => path.basename(filePath) !== "index.js"),
JSON_FILES = find("conf/").filter(fileType("json")),
MARKDOWNLINT_IGNORED_FILES = fs.readFileSync(path.join(__dirname, ".markdownlintignore"), "utf-8").split("\n"),
MARKDOWN_FILES_ARRAY = find("docs/").concat(ls(".")).filter(fileType("md")).filter(file => !MARKDOWNLINT_IGNORED_FILES.includes(file)),
MARKDOWNLINT_IGNORED = fs.readFileSync(path.join(__dirname, ".markdownlintignore"), "utf-8").split("\n"),
Copy link
Member

Choose a reason for hiding this comment

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

Content of .markdownlintignore are gitignore patterns. We can use the ignore library we already have as a dependency to filter out ignored files. That would be easier and more reliable than a custom implementation.

Ideally, we would use a glob library that supports gitignore patterns so that it doesn't traverse ignored directories at all, but that could be another enhancement.

And I'm not sure why are we using markdownlint API here instead of markdownlint CLI which could handle .markdownlintignore by itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

can I try adding the glob library or can we use markdownlint CLI instead of markdownlint API?

Copy link
Member

Choose a reason for hiding this comment

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

I would try with markdownlint CLI. That looks a lot simpler and I don't see any specific reasons to use API for our use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I will update.

@mdjermanovic mdjermanovic changed the title fix: do not lint markdown ignored files chore: do not lint markdown ignored files Jun 25, 2022
@eslint-github-bot eslint-github-bot bot added the chore This change is not user-facing label Jun 25, 2022
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Jun 25, 2022
@bmish
Copy link
Sponsor Member

bmish commented Jun 26, 2022

I'm confused because I do not see any output from markdownlint when I run it locally with npm run lint. In other words, the ignore file works correctly for me. Can you share more about your reproduction?

If something did break here, I would want to investigate what caused it.

@bmish bmish self-requested a review June 26, 2022 04:05
@amareshsm
Copy link
Member Author

Reproducible on windows OS and macOS.

Steps to reproduce:

  1. Install node modules
  2. Run lint command npm run lint
mini.clip.lint.issue.mov

@bmish
Copy link
Sponsor Member

bmish commented Jun 27, 2022

Hey @amareshsm, thanks for that detail. I realized that the reason you are seeing this issue and I'm not is because you have run npm install inside the nested docs/ package.

I put together a simpler implementation of the fix for this in #16060 which I recommend we use instead. Please take a look, and thanks for finding this issue originally.

@mdjermanovic mdjermanovic removed the accepted There is consensus among the team that this change meets the criteria for inclusion label Jun 27, 2022
@mdjermanovic
Copy link
Member

Closing as the problem is fixed by #16060.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 25, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants