[#2833] add unzip to IrodsStorage #44
Conversation
There was a problem hiding this comment.
@sblack-usu I have finished review and added a couple of comments.
storage.py
Outdated
| self.session.run("ibun", None, '-xDzip', zip_file_path, unzipped_folder) | ||
| return unzipped_folder | ||
|
|
||
| def __get_nonexistant_path(self, path): |
There was a problem hiding this comment.
Is there a particular reason to prefix the method with double underscore here? The name mangling resulting from double underscore prefixes may not be good here. I think single underscore prefix to indicate this method is internal method only is sufficient unless I missed something.
There was a problem hiding this comment.
the only reason is a typo. I'll get it updated with one underscore.
storage.py
Outdated
| def unzip(self, zip_file_path): | ||
| """ | ||
| run iRODS ibun command to unzip files | ||
| :param zip_file_path: path of the zipped file to be unzipped |
There was a problem hiding this comment.
It'd be good to add more info in docstring regarding the target folder the zipped file is being unzipped to given that it get_nonexistant_path() is called to create a target folder path implicitly by this unzip method. I have not looked into HydroShare code that calls this unzip method yet, so my question may be answered after I look into that, but I am wondering whether all files unzipped into unzipped folder are linked to Django as part of resource files for the resource so the unzipped files and folders are kind of implicitly controlled/managed by the user.
There was a problem hiding this comment.
yeah, the problem we're running into is when a zipped file is unzipped next to an aggregation, the metadata files for the aggregation get added to django. Unzipping files into a single folder and only adding the folder contents to django is the solution that arose.
There was a problem hiding this comment.
@sblack-usu I have approved this PR, but then another thought come into my mind. I am thinking whether it is worthwhile to make this unzip method a more generic method by providing a target_unzip_path as an optional input argument with a default value set as you did for unzipped_folder currently. I am also thinking ```_get_nonexistant_path()```` may not be needed but instead just override the target path if it already exists. Again I have not got a chance to see how this is being used, so I may misunderstand the intention, but just some thoughts for your consideration.
There was a problem hiding this comment.
The thought occurred to me as well to provide a way to override. However, I decided against implementing it at this time because we are currently not using it. It can be easily implemented when there is a use case.
As for the get_non_existent_path() method, I expect it to be used very little because there most likely won't be a conflict. In the case of a conflict I thought it best to not overwrite files without the user's consent. Considering it's both not likely to happen often and it's much more work to implement a system notifying and handling feedback from the user, I believe this is the best solution with the time constraints we have.
There was a problem hiding this comment.
OK. You can merge this when you are ready then.
There was a problem hiding this comment.
You have me second guessing myself now. Some of the tests in the hydroshare project failed with my changes. Turns out there are some tests out there expecting an exception to be thrown if overwriting files. (http://ci.hydroshare.org:8080/job/hydroshare-pull-requests/1741/testReport/hs_core.tests/TestResourceFileFolderOprsAPI/test_resource_file_folder_oprs/)
I'm going to ask the development channel to see if there is good reason for this, or if I should update the tests to look for the new outcome.
Talked to others about this and we're going to keep the incremented folder name
No description provided.