Improve force overwriting for existing output files #48
Conversation
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
Move the code logics to the front-level trexio_run script + minor cleanup
scemama
reviewed
Nov 10, 2024
src/trexio_tools/trexio_run.py
Outdated
Comment on lines
33
to
44
| def remove_trexio_file(filename:str, overwrite:bool) -> None: | ||
| """Remove the TREXIO file/directory if it exists.""" | ||
| if os.path.exists(filename): | ||
| if overwrite: | ||
| if '*' in filename: | ||
| raise ValueError(f'TREXIO filename {filename} contains * symbol. Are you sure?') | ||
| os.system(f'rm -rf -- {filename} ') | ||
| else: | ||
| raise Exception(f'Output file {filename} already exists but overwrite option is not provided. Consider using the `-w` CLI argument.') | ||
|
|
||
| return | ||
|
|
There was a problem hiding this comment.
Suppose that a user calls trexio_convert -w $HOME/$MY_TREXIO_FILE, this wil call rm -rf -- $HOME/$MY_TREXIO_FILE. If the user made a mistake in his/her script and forgot to define $MY_TREXIO_FILE, it wil make rm -rf $HOME/. :-(
It would be safer to try to open the trexio file first and check something inside it like the trexio metadata, so that we are sure that the thing we remove is indeed a trexio file.
There was a problem hiding this comment.
Done, please have a look.
scemama
requested changes
Nov 10, 2024
|
Great idea! Will do that. |
scemama
approved these changes
Nov 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Mainly moved the file removal logic to the high-level
trexio_run.pyscript and fixed some inconsistencies. Now it callsrm -rfso it should work for both TEXT and HDF5 back ends unlike the previous version that was printing an error message for TEXT TREXIO files.