Skip to content

feat: add support for custom name parameter to includeIgnoreFile #211

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

Merged
merged 5 commits into from
Jun 3, 2025

Conversation

lumirlumir
Copy link
Member

Prerequisites checklist

What is the purpose of this pull request?

Hello,

Currently, the globalIgnores() helper function supports a custom name property in its config object.

image

However, the includeIgnoreFile() helper function, which has similar behavior to globalIgnores(), does not support a custom name for its config object. This prevents users from using a custom name in that context.

So, in this PR, I've added support for a custom name parameter to includeIgnoreFile().

I've also added tests and missing JSDoc type hints to globalIgnores().

What changes did you make? (Give an overview)

Related Issues

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

@lumirlumir lumirlumir requested a review from mdjermanovic May 27, 2025 11:50
nzakas
nzakas previously approved these changes May 27, 2025
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Would like @mdjermanovic to review before merging.

Just a note: When adding public-facing functionality, it's better to open an issue first to gather feedback before sending a PR. That way, we can make sure the team is aligned before moving on to implementation.

@nzakas nzakas moved this from Needs Triage to Second Review Needed in Triage May 27, 2025
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.

Can you also update the documentation for this function in the README?

@lumirlumir
Copy link
Member Author

lumirlumir commented May 29, 2025

@nzakas I'll keep that in mind!


@mdjermanovic I've added a new commit 80cf734!

mdjermanovic
mdjermanovic previously approved these changes May 29, 2025
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! Leaving open for @nzakas to verify the latest changes.

@lumirlumir
Copy link
Member Author

@nzakas Thanks! I've added a new commit d414030.

@lumirlumir lumirlumir requested review from nzakas and mdjermanovic May 31, 2025 06:12
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! Leaving open for @nzakas to verify latest changes.

Copy link
Contributor

@snitin315 snitin315 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!

Copy link
Member

@nzakas nzakas 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.

@nzakas nzakas merged commit 3e18175 into main Jun 3, 2025
18 checks passed
@nzakas nzakas deleted the feat-add-custom-name-param-to-include-ignore-file branch June 3, 2025 19:29
@github-project-automation github-project-automation bot moved this from Second Review Needed to Complete in Triage Jun 3, 2025
@github-actions github-actions bot mentioned this pull request Jun 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

4 participants