-
Notifications
You must be signed in to change notification settings - Fork 256
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 two-pass encoding and create basic CLI tests #2209
Conversation
6210c17
to
8bf02ab
Compare
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.
Seems fine. Thank you for preparing the test.
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 overall, thanks for the fix and for the tests.
fn get_tempfile_path(extension: &str) -> PathBuf { | ||
let mut path = temp_dir(); | ||
let filename = | ||
thread_rng().sample_iter(&Alphanumeric).take(12).collect::<String>(); |
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.
Might be personal preference, but I think tests should avoid using random values. A second opinion would be nice.
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.
We could use non-random filenames, since the directory is already temporary
8bf02ab
to
be785d7
Compare
data | ||
} | ||
|
||
fn get_tempfile_path(extension: &str) -> PathBuf { |
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 may use tempfile maybe?
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 looked into that, but tempfile doesn't have an option to just generate a filename--it also creates the file. For this, we just want a filename in the temp directory that rav1e can write to.
81576ec
to
11dfffb
Compare
This reverts commit 45ef0af.
11dfffb
to
c7175d5
Compare
Closes #2208