Databases: add support for zip archiving #97
Merged
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
lemonsaurus
requested changes
Jun 9, 2021
There was a problem hiding this comment.
Thanks for the PR, Akarys!
A few problems:
- Handler is not well enough documented. Please add some block comments explaining what you're doing.
- I'm not sure about calling this storage type zip. Sure, we're zipping the backup, but we're storing it locally. So I would argue that semantically, it makes more sense for the storage type to be something like "local storage". The fact that we zip it is an implementation detail.
lemonsaurus
requested changes
Jun 9, 2021
README.md
Outdated
| @@ -277,7 +277,7 @@ databases: | |||
|
|
|||
| ## Databases | |||
|
|
|||
| Right now, this app supports **MongoDB**, **PostgreSQL 13**, **MariaDB**, **Redis** and **Zip archiving**. If | |||
| Right now, this app supports **MongoDB**, **PostgreSQL 13**, **MariaDB**, **Redis** and **LocalStorage archiving**. If | |||
There was a problem hiding this comment.
this feels weird, and may be confused with localStorage, the HTML5 feature.
Let's just make it local storage archiving without capital letters.
README.md
Outdated
| @@ -371,14 +371,21 @@ Identifiers can be any string of your choosing. | |||
| port: "6379" | |||
| ``` | |||
|
|
|||
| ### Zip archiving | |||
| ### Local Storage | |||
| elif file.is_dir(): | ||
| directories.append(file) | ||
| for file in path.rglob("*"): | ||
| if file.is_file(): |
There was a problem hiding this comment.
this line irks me a bit. why did we name the variable file if we have to check to make sure it is a file? that's why my suggestion had subpath, but I'm open to other more "zoomed out" alternatives. I just don't think we should name something file and then check if it's a file.
Akarys42
force-pushed
the
zip-files
branch
2 times, most recently
from
787587e
to
75f293e
Compare
Akarys42
changed the title
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.
Closes #87
This commit adds support for recursively storing a local file as a Zip file.