3537: End plugin model necromancy#3538
Conversation
…ved to ensure sasview does not resurrect them
…nd the reasoning for the change
| # Once the file is copied, the old file should be removed from the system to ensure. | ||
| # I did not change the shutil to a move, to ensure files that exist in both locations aren't overlooked. | ||
| if old_path.exists(): | ||
| os.remove(old_path) |
There was a problem hiding this comment.
There are some assumptions here:
The old file is correctly copied, and it is just a single file (keep in mind that os.remove() won't remove directories or links)
The os.remove doesn't throw - a dangerous assumption, as the exception will then percolate with unexpected results
Maybe be a bit more defensive?
for old_path, new_path in location_map.items():
if old_path.exists() and not new_path.exists():
shutil.copy2(old_path, new_path)
# Ensure the file was copied successfully before removing the original
if old_path.exists():
try:
os.remove(old_path)
except Exception as e:
logging.error(f"Failed to remove {old_path}: {e}")There was a problem hiding this comment.
I purposely left the second if statement outside the first. If a user had a model from v5 that got copied to the new location in v6, it now exists in both locations, but should only be in the new locale. The check inside misses the case I am trying to correct for. My new proposed scheme to cover the use cases this is meant to capture:
for old_path, new_path in location_map.items():
# Move the file to the new location instead of copying (fixes future issues)
if old_path.exists() and not new_path.exists():
shutil.move(old_path, new_path)
# Files copied to the new location in previous versions may still be hanging around. Delete them.
if old_path.exists() and new_path.exists():
try:
os.remove(old_path)
except Exception as e:
logging.error(f"Failed to remove {old_path}: {e}")
rozyczko
left a comment
There was a problem hiding this comment.
I'm approving this, but please consider the comments, especially error handling.
…case where both files exist is handles properly
|
@rozyczko, let me know if the changes I made address your concerns, or merge if you think this is ready |
|
looks good! |
Description
This ensures, once files are copied from ~/.sasview/ to the new locations, the original file is removed from the system. The most important aspect of this is to ensure plugin models deleted through the plugin model manager do not reappear later.
This branch was based off 3522-example-data-branch, so I currently have this as a chained PR.
Fixes #3537
How Has This Been Tested?
Locally, all plugin models were wiped away in my ~/.sasview/plugin_models/ directory.
Review Checklist:
Documentation (check at least one)
Installers
Licensing (untick if necessary)