-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix(ext/node): add truncate
method to the FileHandle
class
#27389
Merged
kt3k
merged 6 commits into
denoland:main
from
filipstev:filipstev/truncate_to_file_handle
Dec 20, 2024
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
d16b1b6
Add truncate method to FileHandle class
filipstev 0ee87c5
Rerun jobs
filipstev 6bbf61c
Merge branch 'main' into filipstev/truncate_to_file_handle
filipstev 2dc82e2
stop exposing fs.promises.ftruncate
filipstev 436a9bb
lint code
filipstev 6a12414
trigger ci
filipstev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ export const open = fsPromises.open; | |
export const opendir = fsPromises.opendir; | ||
export const rename = fsPromises.rename; | ||
export const truncate = fsPromises.truncate; | ||
export const ftruncate = fsPromises.ftruncate; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. Let's not expose this |
||
export const rm = fsPromises.rm; | ||
export const rmdir = fsPromises.rmdir; | ||
export const mkdir = fsPromises.mkdir; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,6 +73,10 @@ export class FileHandle extends EventEmitter { | |
} | ||
} | ||
|
||
truncate(len?: number): Promise<void> { | ||
return fsCall(promises.ftruncate, this, len); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably we need to import |
||
} | ||
|
||
readFile( | ||
opt?: TextOptionsArgument | BinaryOptionsArgument | FileOptionsArgument, | ||
): Promise<string | Buffer> { | ||
|
@@ -85,11 +89,7 @@ export class FileHandle extends EventEmitter { | |
length: number, | ||
position: number, | ||
): Promise<WriteResult>; | ||
write( | ||
str: string, | ||
position: number, | ||
encoding: string, | ||
): Promise<WriteResult>; | ||
write(str: string, position: number, encoding: string): Promise<WriteResult>; | ||
write( | ||
bufferOrStr: Uint8Array | string, | ||
offsetOrPosition: number, | ||
|
@@ -120,16 +120,10 @@ export class FileHandle extends EventEmitter { | |
const encoding = lengthOrEncoding; | ||
|
||
return new Promise((resolve, reject) => { | ||
write( | ||
this.fd, | ||
str, | ||
position, | ||
encoding, | ||
(err, bytesWritten, buffer) => { | ||
if (err) reject(err); | ||
else resolve({ buffer, bytesWritten }); | ||
}, | ||
); | ||
write(this.fd, str, position, encoding, (err, bytesWritten, buffer) => { | ||
if (err) reject(err); | ||
else resolve({ buffer, bytesWritten }); | ||
}); | ||
}); | ||
} | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node doesn't seem having
fs.promises.ftruncate
(No doc entry for it, and also not available in runtime) https://nodejs.org/api/fs.htmlLet's not expose it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You make a great point! I've noticed that
fstat
is also exposed but can't seem to find it in the doc, andfs.promises.fstat
also doesn't seem to be available in the runtime. Is that an issue, and if so, is it something we would like to handle within this PR?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Yes, that seems an issue to me.
Fixing it in this PR is welcome, but that's not absolutely necessary in my opinon. I created an issue for
fs.promises.fstat
for now #27423There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I might take a deeper look into that one as well, see if we might have more to clean up :)
Also, the implementation for
promises.ftruncate
has been cleaned up 🧹