Skip to content

fix(ext/node): add chown method to FileHandle class#27638

Merged
kt3k merged 5 commits intodenoland:mainfrom
mstysk:fs_handle_chown
Jan 15, 2025
Merged

fix(ext/node): add chown method to FileHandle class#27638
kt3k merged 5 commits intodenoland:mainfrom
mstysk:fs_handle_chown

Conversation

@mstysk
Copy link
Copy Markdown
Contributor

@mstysk mstysk commented Jan 12, 2025

Add a chown method to the FileHandle class #25554.

This is my first PR. if there is anything missing, please let me know so I can fix it.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jan 12, 2025

CLA assistant check
All committers have signed the CLA.

@mstysk
Copy link
Copy Markdown
Contributor Author

mstysk commented Jan 12, 2025

Windows test seems to have failed

@mstysk
Copy link
Copy Markdown
Contributor Author

mstysk commented Jan 12, 2025

Similar to #27522,
chown is also untested on Windows, so I made it ignore it

@kt3k kt3k mentioned this pull request Jan 14, 2025
24 tasks
@kt3k kt3k self-requested a review January 14, 2025 07:19
Comment on lines +233 to +240
try {
await fileHandle.chown(nobodyUid, nobodyGid);
} catch (e) {
assert(
e instanceof Deno.errors.PermissionDenied,
"Expected permissionDenied error",
);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you use assertRejects instead? This passes when await fileHandle.chown(nobodyUid, nobodyGid) doesn't throw

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't know about assertRejects. I will take it.

@kt3k kt3k changed the title feat(node/fs): add a chown method to the FileHandle class fix(node/fs): add a chown method to the FileHandle class Jan 14, 2025
Signed-off-by: Yoshiya Hinosawa <stibium121@gmail.com>
@mstysk
Copy link
Copy Markdown
Contributor Author

mstysk commented Jan 15, 2025

test release linux-x86_64 is failed. I don't understand why this test fails.

image

@kt3k
Copy link
Copy Markdown
Contributor

kt3k commented Jan 15, 2025

Looks like a flaky failure. I'll rerun it

@mstysk
Copy link
Copy Markdown
Contributor Author

mstysk commented Jan 15, 2025

CI is passed!

Copy link
Copy Markdown
Contributor

@kt3k kt3k 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 for your contribution!

@kt3k kt3k merged commit b22a50c into denoland:main Jan 15, 2025
@kt3k kt3k changed the title fix(node/fs): add a chown method to the FileHandle class fix(ext/node): add chown method to FileHandle class Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants