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.

Sign up for GitHub

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

Jump to bottom

path: add matchesGlob method #52881

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

path: add matchesGlob method #52881

wants to merge 4 commits into from
+99 −0

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented May 7, 2024 •
edited

Fixes #52779

This PR adds path.matchesGlob(path, pattern), which checks whether a glob (pattern) matches the input path (path).

@RedYetiDev RedYetiDev added path Issues and PRs related to the path subsystem. needs-ci PRs that need a full CI run. labels May 7, 2024
@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 7, 2024 •
edited

"Label PRs / label" appears to have crashed, so I manually labeled the PR.

@nodejs/actions https://github.com/nodejs/node/actions/runs/8992208708/job/24701514516

GitHub Actions has encountered an internal error when running your job.

MoLow
MoLow reviewed May 8, 2024
View reviewed changes
doc/api/path.md Outdated Show resolved Hide resolved
MoLow
MoLow approved these changes May 8, 2024
View reviewed changes
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

Imlementation LGTM. not sure about the naming though

@MoLow MoLow added semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. labels May 8, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 8, 2024

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @MoLow.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

targos
targos previously requested changes May 8, 2024
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

Please make it experimental.

@RedYetiDev RedYetiDev changed the title path: add glob method path: add matchGlob method May 8, 2024
@RedYetiDev RedYetiDev requested a review from targos May 8, 2024 19:02
MoLow
MoLow reviewed May 9, 2024
View reviewed changes
lib/path.js Show resolved Hide resolved
@RedYetiDev @MoLow
Update path.js
ba21084 
Co-authored-by: Moshe Atlow <moshe@atlow.co.il>
benjamingr
benjamingr approved these changes May 9, 2024
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

LGTM. (fix the missing import)

@targos targos dismissed their stale review May 9, 2024 11:38

Addressed

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2024
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels May 9, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 9, 2024

Failed to start CI
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/9017575572

@RedYetiDev RedYetiDev added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels May 9, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59054/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59063/

tniessen
tniessen reviewed May 10, 2024
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

In the context of patterns, in the JS ecosystem, match() typically returns a match object (e.g., String.prototype.match). This should probably be matchesGlob() instead if it returns a boolean.

@RedYetiDev RedYetiDev changed the title path: add matchGlob method path: add matchesGlob method May 10, 2024
@RedYetiDev RedYetiDev requested a review from tniessen May 10, 2024 19:20
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label May 11, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 11, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59128/

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59156/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marco-ippolito marco-ippolito marco-ippolito left review comments

@benjamingr benjamingr benjamingr approved these changes

@MoLow MoLow MoLow approved these changes

@targos targos Awaiting requested review from targos

@tniessen tniessen Awaiting requested review from tniessen

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. path Issues and PRs related to the path subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replacement for minimatch
7 participants
@RedYetiDev @nodejs-github-bot @benjamingr @targos @tniessen @MoLow @marco-ippolito