-
Notifications
You must be signed in to change notification settings - Fork 85
Unify incremental_create_output_dir
#2870
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
base: zdobnikau/execute-save-outputs
Are you sure you want to change the base?
Unify incremental_create_output_dir
#2870
Conversation
41c979a to
037b8ef
Compare
a07fae6 to
d5ccd3c
Compare
create-output-dirincremental_create_output_dir
0ba8159 to
a267865
Compare
0befe5e to
100b832
Compare
58546ae to
f62beb5
Compare
utils/scarb-execute-utils/Cargo.toml
Outdated
| @@ -0,0 +1,10 @@ | |||
| [package] | |||
| name = "scarb-execute-utils" | |||
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.
Nit: Maybe just scarb-fs-utils? 🤔 Seems general enough to me.
| pub const EXECUTE_PROGRAM_OUTPUT_FILENAME: &str = "program_output.txt"; | ||
| pub const EXECUTE_PRINT_OUTPUT_FILENAME: &str = "stdout_output.txt"; |
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'd just keep them where they are used.
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 moved them here so we could re-use these both in scarb execute and scarb doc in #2876.
Can define these in both places, but not sure why?
IMO it's reasonable to keep this crate as scarb execute utils rather then general. If we do, it makes sense to keep those here.
utils/scarb-execute-utils/src/lib.rs
Outdated
| /// The dir name is `execution{N}` with the lowest `N` without existing dir. | ||
| /// | ||
| /// Returns the path to the created directory and corresponding `N`. | ||
| pub fn incremental_create_execution_output_dir(path: &Utf8Path) -> Result<(Utf8PathBuf, usize)> { |
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.
Let's just take execution as param.
| pub fn incremental_create_execution_output_dir(path: &Utf8Path) -> Result<(Utf8PathBuf, usize)> { | |
| pub fn incremental_create_dir_unique(path: &Utf8Path, name: &str) -> Result<(Utf8PathBuf, usize)> { |
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'm not sure we expect incremental_create_dir_unique to be used in any other contexts? 🤔
If we don't, it seems reasonable to me to keep this crate scarb execute-focused.
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.
it seems reasonable to me to keep this crate scarb execute-focused
I'd look at this from the other side, that is, it makes sense to me to define some crate for how Scarb interacts with the filesystem. E.g. canonicalization helpers from internal/fsx.rs could go there in the future - this is not Scarb specific, and would make sense in the extensions as well.
f62beb5 to
84d2481
Compare
2f3fb9a to
f0cbf01
Compare
Stack
scarb executeoutputs #2869incremental_create_output_dir#2870should_panicandcompile_faildoc-test attributes #2893