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

Export has scary overwrite behaviour. #66

Closed
will-ca opened this issue Nov 21, 2022 · 3 comments
Closed

Export has scary overwrite behaviour. #66

will-ca opened this issue Nov 21, 2022 · 3 comments
Labels
bug Something isn't working urgent

Comments

@will-ca
Copy link

will-ca commented Nov 21, 2022

Describe the bug

  • Apparently -e always saves to the user home? So if you cd to a subdirectory hoping to contain everything, then it still writes to your top-level user home.
  • When the output file $HOME/<PROFILENAME>.knsv already exists, then Konsave just straight-up overwrites it.
  • When the seemingly unrelated path $HOME/<PROFILENAME>.zip exists, then Konsave also just straight-up overwrites (and then removes) that too.
  • Konsave does not give you the opportunity to specify output filename either.
  • So the default behaviour of konsave -e is apparently to immediately overwrite two seemingly unrelated filepaths that the user didn't specify, in a location that the user also didn't specify, which also happens to be the top-level user home.
  • When the output already exists and is a directory, Konsave creates the file <PROFILENAME>.zip inside it instead.
  • When the output already exists and <PROFILENAME>.zip also already exists in it, then Konsave finally fails instead of overwriting (but I think not deliberately?).
  • If the profile name includes capitals, the "Successfully exported" message printed at the end switches them to lower-case instead, meaning the path reported to the user is wrong.

To reproduce
Use the konsave -e feature.

Expected behavior
Tools should never overwrite user data unless explicitly instructed to do so.

E.G.:

  • Do mktemp (Python: probably import tempfile) for the scratch file instead of putting it next to the output.
  • Write to os.getcwd() instead of user home.
  • Check for existing file before rename, and either fail or prompt (possibly depending on -f flag) instead of overwriting.
  • Possibly allow/require user to explicitly specify output filename/path.
@Prayag2
Copy link
Owner

Prayag2 commented Nov 24, 2022

This actually sounds scary now that I think about it. I'll definitely consider all of your suggestions to improve Konsave but unfortunately, I have an exam that I have to prepare for (and it's a tough exam). I'll make sure to fix this issue as soon as I get time after my exams. Thanks for reporting :)

@Prayag2 Prayag2 added bug Something isn't working urgent labels Nov 24, 2022
@gadgieOps
Copy link
Contributor

Hi @Prayag2 ,

I've been finding great use for this project, thank you for making the code publicly available. I share @will-ca 's concerns. I have made some changes in a fork and will submit a pull request for you to review. I'll explain the changes in the PR, they're not fool proof, but have marked improvements on handling of exports.

@Prayag2
Copy link
Owner

Prayag2 commented Jan 30, 2023

Fixed in #72 by @gadgieOps :)

@Prayag2 Prayag2 closed this as completed Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working urgent
Projects
None yet
Development

No branches or pull requests

3 participants