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

feat: Add includeIgnoreFile() method #47

Merged
merged 6 commits into from
Jun 10, 2024
Merged

feat: Add includeIgnoreFile() method #47

merged 6 commits into from
Jun 10, 2024

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Jun 6, 2024

Prerequisites checklist

What is the purpose of this pull request?

Add a includeIgnoreFile() function.

What changes did you make? (Give an overview)

  • Added includedIgnoreFile() function that is exported from @eslint/compat
  • Added convertIgnorePatternToMinimatch() function that is exported from @eslint/compat (purposely undocumented because I'm not sure how end users would use this)
  • Updated README for @eslint/compat
  • Switched @eslint/migrate-config to use the convertIgnorePatternToMinimatch() function from @eslint/compat instead of duplicating that functionality
  • Rearranged the order in which packages are released so @eslint/migrate-config comes after @eslint/compat due to the new dependency on it

Note: The includeIgnoreFile() function differs from the original issue in that there is no second argument. I'm not sure it's needed and it could complicate the process if the ignore file is in an ancestor directory. For this first release, I'd like to see what kind of feedback we get as-is.

Related Issues

fixes #44

Is there anything you'd like reviewers to focus on?

import { includeIgnoreFile } from "@eslint/compat";

export default [
includeIgnoreFile(".gitignore"),
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to recommend passing an absolute path here. Relative paths are resolved relative to process.cwd(), so whether this will work or not depends on where eslint is run from.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right. 🤦 Second time this week I forgot that fact.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

Just one typo, and there's also a merge conflict.

packages/compat/README.md Outdated Show resolved Hide resolved
nzakas and others added 5 commits June 10, 2024 11:12
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit b5f74ed into main Jun 10, 2024
14 checks passed
@mdjermanovic mdjermanovic deleted the issue44 branch June 10, 2024 16:10
@github-actions github-actions bot mentioned this pull request Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Change Request: Utility function to read gitignore files
2 participants