Closed jbogarin closed 5 years ago
@jbogarin I like the changes. I'm OK with the breaking change because we were returning the resulting filename before and that will not change. Users should rely on the output of the function instead of assuming it will be in a specific format. I would like to ask you to make the following changes:
1) Since make_archive
returns a filename, let's change self._compress_folder(file_format)
to filename = self._compress_folder(file_format)
and then we don't need this block:
filename = ""
if file_format == "gztar":
filename = f"{self.archive_folder_name}.tar.gz"
elif file_format == "zip":
filename = f"{self.archive_folder_name}.zip"
else:
filename = f"{self.archive_folder_name}.tar.gz"
2) Change the docstring for file_format
to say "file_format: Archive format as supported by shutil.make_archive"
Felipe,
I made the changes suggested by you. Please let me know if you think more changes are needed.
For some operating systems (Windows), the tgz file format is not as common; zip file is better. I changed the code from using the tarfile module to use shutil.make_archive and included an extra option named file_format.
However, this is a breaking change as this code changes the extension from tgz to tar.gz which is the one that make_archive creates.
If we need to keep backward compatibility, it would be necessary to include a rename functionality.
I'm just creating the pull request to have the discussion around this change.