-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Implement deno.truncate #805
Conversation
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.
Looks good, 2 comments
src/handlers.rs
Outdated
if !deno.flags.allow_write { | ||
return odd_future(permission_denied()); | ||
} | ||
let msg = base.msg_as_truncate().unwrap(); |
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.
Remove this extra space (you might want to run ./tools/format.py
and ./tools/lint.py
)
src/handlers.rs
Outdated
let len = msg.len(); | ||
Box::new(futures::future::result(|| -> OpResult { | ||
debug!("handle_mkdir {}", name); | ||
debug!("handle_mkdir {}", len); |
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.
I believe you could do
debug!("handle_truncate {} {}", name, len);
Also, it should be handle_truncate
instead of handle_mkdir
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.
Thanks you
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.
Looks good - thanks!
Just a few questions...
js/truncate_test.ts
Outdated
const enc = new TextEncoder(); | ||
const d = enc.encode("Hello"); | ||
const filename = deno.makeTempDirSync() + "/test_truncateSync.txt"; | ||
deno.writeFileSync(filename, d, 0o666); |
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.
Remove optional mode argument 0o666
, it's not relevant to this test.
assertEqual(data.length, 0); | ||
|
||
// delete test txt | ||
deno.removeSync(filename); |
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.
I would remove all the comments here - it's self explanatory. Also remove blank links where appropriate.
src/handlers.rs
Outdated
let len = msg.len(); | ||
Box::new(futures::future::result(|| -> OpResult { | ||
debug!("handle_truncate {} {}", name, len); | ||
let mut options = fs::OpenOptions::new(); |
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.
What about using this https://doc.rust-lang.org/std/fs/struct.OpenOptions.html#method.truncate
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.
I believe that method would truncate the whole file to length 0
debug!("handle_truncate {} {}", name, len); | ||
let mut options = fs::OpenOptions::new(); | ||
let f = options.write(true).open(name)?; | ||
f.set_len(len as u64)?; |
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.
This is how you truncate in rust? This seems odd.
Can you link me to documentation? I can't find anything on this....
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.
I believe he meant this call: File.set_len
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.
Thanks
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.
@ry Here is link. https://doc.rust-lang.org/std/fs/struct.File.html#method.set_len In order for use set_len
, write access must be used.
js/unit_tests.ts
Outdated
@@ -17,5 +17,8 @@ import "./symlink_test.ts"; | |||
import "./platform_test.ts"; | |||
import "./text_encoding_test.ts"; | |||
import "./trace_test.ts"; | |||
import "./truncate_test.ts"; | |||
|
|||
import "../website/app_test.js"; |
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.
Don't include this change
Hi - I’ve landed a bunch of changes to the handlers structure including blocking! macros. Can you rebase/update this PR to use them? |
@ry Sure. |
src/handlers.rs
Outdated
let msg = base.msg_as_truncate().unwrap(); | ||
let name = msg.name().unwrap(); | ||
let len = msg.len(); | ||
Box::new(futures::future::result(|| -> OpResult { |
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.
Please use blocking!()
here. You'll see examples above.
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.
@ry Done.
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.
LGTM - thank you!
- Improve fetch headers (denoland#853) - Add deno.truncate (denoland#805) - Add copyFile/copyFileSync (denoland#863) - Limit depth of output in console.log for nested objects, and add console.dir (denoland#826) - Guess extensions on extension not provided (denoland#859) - Renames: deno.platform -> deno.platform.os deno.arch -> deno.platform.arch - Upgrade TS to 3.0.3 - Add readDirSync(), readDir() - Add support for TCP servers and clients. (denoland#884) Adds deno.listen(), deno.dial(), deno.Listener and deno.Conn.
- Improve fetch headers (#853) - Add deno.truncate (#805) - Add copyFile/copyFileSync (#863) - Limit depth of output in console.log for nested objects, and add console.dir (#826) - Guess extensions on extension not provided (#859) - Renames: deno.platform -> deno.platform.os deno.arch -> deno.platform.arch - Upgrade TS to 3.0.3 - Add readDirSync(), readDir() - Add support for TCP servers and clients. (#884) Adds deno.listen(), deno.dial(), deno.Listener and deno.Conn.
Node:
fs.truncate(path[, len], callback)
Go:
func Truncate(path string, length int64) (err error)
Python:
os.truncate(path, length)
Rust:
pub fn set_len(&self, size: u64) -> Result<()>