GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fs: correctly pass dirent to exclude `withFileTypes` by avivkeller · Pull Request #53823 · nodejs/node

Conversation

@avivkeller

@avivkeller avivkeller commented Jul 12, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Fixes #53821

Fixes an issue where some calls to #exclude did not pass the dirent when withFileTypes: true

CC @targos

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Jul 12, 2024
@avivkeller avivkeller mentioned this pull request Jul 12, 2024
fsPromises.glob doesn't call exclude with dirent when called withFileTypes: true #53821
Closed
benjamingr
benjamingr reviewed Jul 12, 2024
View reviewed changes
Comment thread test/parallel/test-fs-glob.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr approved these changes Jul 12, 2024
View reviewed changes
@targos

targos commented Jul 12, 2024

Copy link
Copy Markdown
Member

direct -> dirent
avivkeller reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@avivkeller
fs: correctly pass dirent to exclude withFileTypes
91579a9
@avivkeller avivkeller changed the title fs: correctly pass direct to exclude withFileTypes fs: correctly pass dirent to exclude withFileTypes Jul 12, 2024
targos
targos approved these changes Jul 12, 2024
View reviewed changes
@avivkeller
Update test-fs-glob.mjs
59e8fce
@avivkeller

avivkeller commented Jul 12, 2024

Copy link
Copy Markdown
Member Author

@benjamingr Ive undone the common.mustCall, as the exclude function isn't called for every glob pattern that is being checked.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@avivkeller
avivkeller marked this pull request as draft July 12, 2024 21:36
@avivkeller

avivkeller commented Jul 12, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Removed the last common.mustCall, so this should be good, assuming all tests pass
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@avivkeller
Update test-fs-glob.mjs
0657e79
@avivkeller
avivkeller marked this pull request as ready for review July 12, 2024 21:37
@avivkeller

avivkeller commented Jul 13, 2024

Copy link
Copy Markdown
Member Author

All tests pass, can someone approve and run a Jenkins ci?

Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow
MoLow approved these changes Jul 14, 2024
View reviewed changes
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 14, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 14, 2024
@nodejs-github-bot

nodejs-github-bot commented Jul 14, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60314/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 15, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60324/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 17, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60378/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@avivkeller

avivkeller commented Jul 17, 2024

Copy link
Copy Markdown
Member Author

All CIs passed :-). The two "failed" tests are just stopped linters.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Jul 17, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Jul 17, 2024
@nodejs-github-bot

nodejs-github-bot commented Jul 17, 2024

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/53823
✔  Done loading data for nodejs/node/pull/53823
----------------------------------- PR info ------------------------------------
Title      fs: correctly pass dirent to exclude `withFileTypes` (#53823)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     RedYetiDev:fix-dirent -> nodejs:main
Labels     fs, needs-ci, commit-queue-squash
Commits    3
 - fs: correctly pass dirent to exclude `withFileTypes`
 - Update test-fs-glob.mjs
 - Update test-fs-glob.mjs
Committers 2
 - RedYetiDev <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/53823
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53823
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 12 Jul 2024 14:00:55 GMT
   ✔  Approvals: 3
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/53823#pullrequestreview-2175131131
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/53823#pullrequestreview-2175259315
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/53823#pullrequestreview-2176695952
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-07-17T06:37:36Z: https://ci.nodejs.org/job/node-test-pull-request/60378/
- Querying data for job/node-test-pull-request/60378/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/9974836012
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow removed the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label Jul 17, 2024
@avivkeller

avivkeller commented Jul 17, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@MoLow can you rerun the stopped actions so the bot counts the GitHub CI as a pass? (lint-readme and lint-pr-url)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow pushed a commit that referenced this pull request Jul 17, 2024
@avivkeller @MoLow
fs: correctly pass dirent to exclude withFileTypes
5090166 
PR-URL: #53823
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@MoLow

MoLow commented Jul 17, 2024

Copy link
Copy Markdown
Member

Landed in 5090166
avivkeller reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow closed this Jul 17, 2024
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
@avivkeller @ehsankhfr
fs: correctly pass dirent to exclude withFileTypes
06e710f 
PR-URL: nodejs#53823
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
targos pushed a commit that referenced this pull request Jul 28, 2024
@avivkeller @targos
fs: correctly pass dirent to exclude withFileTypes
b377b93 
PR-URL: #53823
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jul 30, 2024
v22.6.0 proposal #54123
Merged
@targos targos added the dont-land-on-v20.x label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes
@MoLow MoLow MoLow approved these changes
+1 more reviewer
@targos targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

commit-queue-squash PRs the Commit Queue should land as one squashed commit. fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fsPromises.glob doesn't call exclude with dirent when called withFileTypes: true

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@avivkeller @targos @nodejs-github-bot @MoLow @benjamingr
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.