Skip to content
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

ENH: Update main.cpp #49

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ENH: Update main.cpp #49

wants to merge 1 commit into from

Conversation

User-DK
Copy link
Contributor

@User-DK User-DK commented Jul 19, 2024

This update ensures that there are no inconsistencies when the ouput directory has already been build then it just doesn't appends but first removes and then writes to the output directory.

Tests Passed: Yes

This update ensures that there are no inconsistencies when the ouput directory has already been build then it just doesn't appends but first removes and then writes to the output directory.

Tests Passed: Yes
@MSallermann
Copy link
Member

MSallermann commented Jul 30, 2024

It's definitely a good idea to improve the consistency of repeated runs!

I wonder if this behavior might be a bit unsafe though. Given sufficient user error, it might lead to unintentional deletions of arbitrary directories.

Maybe the default should be to just abort the run if the output directory already exists and is not empty.

We could implement "force" flag -f to optionally remove the output directory before the run. What do you think?

@User-DK
Copy link
Contributor Author

User-DK commented Aug 1, 2024

Yes actually, might be a error saying that the output directory exists would also have been sufficient, and deleting without the user consent is also a wrong behaviour so, we can just make it to throw error if output directory already exists and is not empty. So that user can take the further step whether to delete the directory or to make another directory. Is that fine ?

@@ -44,6 +44,7 @@ int main( int argc, char * argv[] )
fmt::print( "Using input file: {}\n", config_file_path.string() );
fmt::print( "Output directory path set to: {}\n", output_dir_path.string() );

fs::remove_all(output_dir_path);// Remove any existing output directory
Copy link
Member

Choose a reason for hiding this comment

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

Make this conditional on a --force flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants