0

Given a method that is widely used and has a void return type:

from somepackage import download_model
from somepackage import get_filename

def download(name, download_path):
   response = download_model(name, download_path)
   file_name = response.filename
   file_path = model_download_folder + file_name
   if file_name.endswith(".tar.gz"):
       with tarfile.open(file_path, "r:gz") as tar_file:
           tar_file.extractall(path=download_path)

This method downloads a folder or a tar file. Then, if the downloaded file is a tar, it extracts it, and the suffix had the file name. For example, if download is called with download_path=/some/path/ and response.filename is 'model.tar.gz' the actual content of the tar will be in /some/path/model/.

This method is currently hiding the filename in the response. So far, the filename was hard-coded and fixed for all downloads. Recently new models was added and now the filename might be anything. The caller to download in this case, should be able to get the path where the files were extracted (e.g. /some/path/model/).

Alternatives:

  1. Change download to return the response/filename
  2. Store the response in a global field and then add another method to get it
  3. Call to get_filename (which wasn't used so far) to get the filename

Assume that backwards compatibility is a constraint. Which one of these options would you recommend and why.

Improve this question
16
  • I'm not sure I understand what it's doing now. So the method currently has no 'return' statement? What does it actually do? Commented Jun 29, 2021 at 15:33
  • why can't you just return the filename? Commented Jun 29, 2021 at 16:27
  • "there is a new requirement and the filename needs to be accessible after calling download" - that's not exactly a full description of a requirement; it's not a "why" (a reason behind the requirement), but more a "how" (it's a partial description in terms of how you'd go about implementing it). In other words, the requirement is likely something along the lines of: "if the dwnld was successful, something needs to happen (and the filename is needed for that)". So I suppose you should return the response to client code, which should check if it's valid (or something like that). Commented Jun 29, 2021 at 17:28
  • Regarding your other options: (2.) is an unstructured, ad hoc access to a global, which is just a terribly bad practice. (3.) get_filename doesn't allow you to check if the download was successful (or whatever response allows for), and given that the filename is already extracted in response.filename, isn't DRY. Also, it doesn't conceptually correspond to what the code is doing at this level (it's not trying to extract a filename from some path, but to do something related to this particular download). Commented Jun 29, 2021 at 17:31
  • Is this a specific technical question that would be better on StackOverflow? or is this more of a philosophical question of what works best? Commented Jun 29, 2021 at 18:45

0

Reset to default

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.