Skip to content

Conversation

@eryue0220
Copy link
Contributor

This pr is part of #6255, which aims to add remove and removeSync api.

@eryue0220 eryue0220 requested a review from kt3k as a code owner February 21, 2025 08:59
@github-actions github-actions bot added the fs label Feb 21, 2025
@codecov
Copy link

codecov bot commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 37.25490% with 32 lines in your changes missing coverage. Please review.

Project coverage is 95.85%. Comparing base (e02e89f) to head (32729d9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
fs/unstable_remove.ts 37.25% 32 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6438      +/-   ##
==========================================
- Coverage   95.93%   95.85%   -0.08%     
==========================================
  Files         568      569       +1     
  Lines       42586    42637      +51     
  Branches     6397     6400       +3     
==========================================
+ Hits        40853    40869      +16     
- Misses       1694     1729      +35     
  Partials       39       39              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

await remove(tempDir);
const existedCheckAgain = await stat(tempDir);
assert(existedCheckAgain.isDirectory === false);
} catch {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be finally. Currently this catches the error thrown from await stat(tempDir) and swallows it (The last assertion doesn't seem working).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Thank you~

await remove(tempDir);
const fsStat = await stat(tempDir);
assert(fsStat.isDirectory === false);
} catch {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto. This needs to be finally

Suggested change
} catch {
} finally {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

…no_std into feat/fs-unstable-remove-api

* 'feat/fs-unstable-remove-api' of github.com:eryue0220/deno_std:
  test(fs/unstable): remove windows specific paths and fix ci (denoland#6448)
  BREAKING(tar/unstable): fix handling of mode, uid, and gid (denoland#6440)
@eryue0220 eryue0220 marked this pull request as draft March 2, 2025 13:19
@eryue0220 eryue0220 marked this pull request as ready for review March 2, 2025 13:59
…no_std into feat/fs-unstable-remove-api

* 'feat/fs-unstable-remove-api' of github.com:eryue0220/deno_std:
  feat(fs/unstable): add umask (denoland#6454)
  feat(fs/unstable): add utime and utimeSync (denoland#6446)
@eryue0220
Copy link
Contributor Author

It seems could not use finally, the ci will fail because it will throw and error: Error: ENOENT: no such file or directory, stat /var/folders/T/

Copy link
Member

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

@kt3k
Copy link
Member

kt3k commented Mar 3, 2025

It seems could not use finally, the ci will fail because it will throw and error: Error: ENOENT: no such file or directory, stat /var/folders/T/

The error was thrown from rm call in the finally block, which can be avoided by passing force: true option.

@kt3k kt3k merged commit e9b8e0b into denoland:main Mar 3, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants