core/structure: help locate/extract gdpr exports #175
Conversation
|
somewhat related karlicoss/kompress#10 |
|
Hmm, think the failures are unrelated to this PR... ? Not sure, looks like a mypy config error or something Expect there to be a few more commits before this can be squashed |
|
Think thats all for now Could also add |
|
|
||
| if is_zip: | ||
| # make sure we're not mistakenly deleting data | ||
| assert str(searchdir).startswith(str(tdir)), f"Expected the temporary directory for extracting zip to start with the temporary directory prefix ({tdir}), found {searchdir}" |
There was a problem hiding this comment.
The context manager looks good! It also seems safe because of this assert; even though in theory it's doesn't look really necessary in the current version of code, if someone added some code between line 113-115, and it threw exception, it would be possible to delete the actual zip file by accident.
Such condition cleanup arises now and then and I prefer to handle it slightly differently
Although I feel like for such cases it's cleaner to rely on context managers as well. So I'd rewrite it as something like:
@contextmanager
def ctx_zip():
with ZipFile(base.absolute()) as zf, tempfile.TemporaryDirectory(dir=tdir) as td:
# technically, ZipFile ctx manager is not necessarily since we're reading only?
# but still, I guess it's nice to call close()
zf.extractall(path=str(td))
yield Path(td)
@contextmanager
def ctx_dir():
yield base.absolute()
ctx = ctx_zip if is_zip else ctx_dir
with ctx as searchdir:
# rest of code in try section (lines 124-142)
That way all the logic for each case is local and it's easier to reason about it
There was a problem hiding this comment.
Ah right, I knew there was something I could do with yielding back the location to make it generic across dir/zipfile -- agreed it is cleaner, not sure if thats more or less understandable for someone unfamiliar with contextmanagers trying to read this code though
I'm fine leaving it as is if you are
|
Thanks, all looks good overall! Sorry took so long to look, was away. |
|
As an update, integrated this into my.discord; now my discord data is just: (When unzipped, it would have been): With |
|
very cool! mine are nearly not as huge, but still nice to not have to maintain an unpacked view! |
I tried to refactor
get_filesinto something that could handleextracting zipfiles or traversing structures, but the differences between
zipfile.Pathandpathlib.Pathmake it pretty annoying, startspolluting the types and handling that without some hack
of just overwriting/subclassing the type seems difficult, especially
while remaining backwards compatible
The purpose here was to temporarily extract the zipfile
into a temporary directory so other code (like my discord data module) can treat it as a real
filesystem, instead of having to deal with
zipfile/archive codeeverywhere.
I expect to run into the same problem for Google takeout in the future, it feels
easier to handle the extraction here in HPI, and then pass off a real directory
to Google takeout which knows how to parse that directory structure
This is something I had originally wrote just for my
discord module, but I feel like it would be pretty useful
to traverse any sort of gdpr directory structure (especially when
you're not sure if you have one or more gdpr exports, and you
want to merge them)
The context manager transparently unzips into the
core_config.get_tmp_dirfolder (if the passed base is a zipfile), otherwise it recursively searches
for matching relative paths. If you're using cachew, that means it only gets
unzipped once while youre cache is still valid
I think the docstrings are good enough, let me know if theres anything you'd
want to change, particularly maybe about the location of the testdata/tests?
I think I added enough robustness so I don't mistakenly shutil.rmtree stuff,
but I wrote this on a plane with only access to
python3 -c 'import contextlib; help(contextlib)'so would appreciate just a look over that as this is the first contextmanager I've ever written