Downloader in Python

Question

I wrote this code in python and it works fine for me but I do know that the code is not optimised and a lot of refactoring needs to be done. So I need review on how this code can be improved. I started writing this when WGET library was not working for me and I wanted a light weight script for my other projects. I am also thinking of replacing requests library with aiohttp .Being a beginner in this, I look forward for your reviews.

Thank You.

Requirements:- tqdm, requests

Implementation:- downloader.py

import requests
import os
from uuid import uuid4
from urllib.parse import urlparse, unquote
import re
from datetime import datetime
from requests.exceptions import HTTPError, ReadTimeout,InvalidSchema
from requests.adapters import HTTPAdapter
from urllib3.util.retry import Retry
from tqdm import tqdm


class Rget:
  def __init__(self, url, dest=os.getcwd(), filename=None, progress_bar=True, headers=None):
    self.url = url
    self.dest = self.check_if_dir_exist(dest)
    self.filename = filename
    self.progress_bar = progress_bar
    # self.headers = self.fetch_headers(headers)

  def check_if_dir_exist(self, dest):
    """
    Function to check whether the directory exist.
    If Directory is not present it creates one and returns the path.
    """
    if not os.path.exists(dest):
      os.makedirs(dest)
    return dest
  
  def detect_filename(self, url, response):
    """
    Function to autodetect file name from url and content disposition
    headers.
    """
    if not self.filename == None:
      self.filename = self.get_valid_filename(self.filename)
    else:
      if 'filename' in response.headers.get('Content-Disposition'):
        filename = response.headers.get('Content-Disposition') \
          .split('filename=')[1].split(';')[0].replace('"', '')

      else:
        filename = os.path.basename(urlparse(unquote(response.url))[2])        

      self.filename = self.get_valid_filename(filename)
  
  def get_valid_filename(self, filename):
    """
    Return the given string converted to a string that can be used for a clean
    filename. Remove leading and trailing spaces; convert other spaces to
    underscores; and remove anything that is not an alphanumeric, dash,
    underscore, or dot.

    https://github.com/django/django/blob/master/django/utils/text.py
    """
    s = str(filename).strip()
    separator = ' '
    return re.sub(r'(?u)[^-\w.]', separator, s)
  
  def fix_existing_filename(self, filename, dest):
    """
    Function that checks whether the file is already downloaded(exists)
    If already downloaded adds a prefix of current timestamp and returns
    the filename along with proper extension
    """
    name, ext = filename.rsplit('.', 1)
    time = datetime.now().strftime('%m-%d-%Y_%I.%M.%S%p')
    name = name+'_'+time
    return name+'.'+ext
  
  def requests_retry_session(self,
    retries=3,
    backoff_factor=0.3,
    status_forcelist=(500, 502, 504),
    session=None,
    ):
    """
    A high level function that I certainly didnot write
    and I don't remember where I copied it from so if somebody knows whose code
    this is then inform me.

    What it bascially does is it automatically retries the request be it
    HEAD, POST, GET, DELETE for 3 times(defalut) can be changed.
    """
    session = session or requests.Session()
    retry = Retry(
        total=retries,
        read=retries,
        connect=retries,
        backoff_factor=backoff_factor,
        status_forcelist=status_forcelist,
    )
    adapter = HTTPAdapter(max_retries=retry)
    session.mount('http://', adapter)
    session.mount('https://', adapter)
    return session
  
  def download(self):
    """
    Function to download file into a temporary file and rename 
    it to user provided filename or autodetected filename.
    """
    try:
      with self.requests_retry_session().get(self.url, stream=True, timeout=3) as response:
        response.raise_for_status()
        self.detect_filename(self.url, response)
        self.file_size = int(response.headers['Content-Length'].strip())
        with open(os.path.join(self.dest, 'rget_'+str(uuid4())+'.tmp'), 'wb+') as temp:
          with tqdm(
            total = self.file_size,
            initial=0,
            unit='B',
            desc=self.filename,
            ascii=True,
            unit_scale=True,
            unit_divisor=1024,
          ) as progressBar:

            for chunk in response.iter_content(chunk_size=8192):
              temp.write(chunk)
              progressBar.update(len(chunk))

        if os.path.exists(os.path.join(self.dest, self.filename)):
          self.filename = self.fix_existing_filename(self.filename, self.dest)
        os.rename(temp.name, os.path.join(self.dest, self.filename))

      return self.filename

    #* A bit of Exception handling to showoff ;)
    except ReadTimeout:
        return('Maximum Retries reached, Check your internet connection and try again')
    
    except:
      return 'Please check the url and try again'

Usage:-

# importing Rget class from downloader.py
from downloader import Rget

url = 'https://drive.google.com/u/0/uc?id=18dn4ha9Lyb1MqjYEjtRAEA5uEKxjPkwD&export=download'

# Optional parameters like destination and fileName can also be provided
file = Rget(url = url)

# printing the fileName once the file gets downloaded
# since download funtion returns the filename
print(file.download())

Answer 1

First, a couple of style/linting things:

1. You are importing HTTPError and InvalidSchema from requests.exceptions but are not using them.
2. Be consistent about your indentation. 4 spaces is the recommended number by PEP8 and it's OK if you don't want to follow that, but try not to mix 2 and 4 space indentation in the same project, like you're doing inside requests_retry_session()
3. Try to use string formatting instead of concatenating with +. This saves you the trouble of manually converting values to str (like you do with the uuid in download()) and it's also easier to read. Take a look at f-strings if you're on Python 3.6+ (which you should): https://realpython.com/python-f-strings/
4. Do not compare to None with ==. Using the is keyword is the more idiomatic way of doing it. The first line in detect_filename() can be rewritten as if self.filename is not None. See: https://stackoverflow.com/questions/14247373/python-none-comparison-should-i-use-is-or
5. As a general rule, commented code is something we don't need, so you we might as well delete it completely. If you ever need that line back you can always get it from your git history. Because you're using git, right? RIGHT??

Minor, nitpicky things:

1. The last bit of download() uses a bare except, which is usually a bad idea because it catches some exceptions you probably don't want to catch. See: https://stackoverflow.com/questions/54948548/what-is-wrong-with-using-a-bare-except
2. Your docstring for fix_existing_filename() says it checks if the filename already exists, but it doesn't actually do that.
3. In download(), you don't need to open the file as read-write if you don't intend to actually read from it. Setting your open mode to just wb makes it clearer for the reader that you only intend to write to that file.
4. In check_if_dir_exist you don't need the if statement, because you can pass exist_ok=True to os.makedirs and that will automatically create the directory only if it doesn't exist. In fact, I would get rid of this method entirely because you can just do everything in one line.
5. Instead of generating a temporary file name yourself, take a look at the tempfile module in the standard library. Not only does it solve the same problem you did with uuid4, but it's also a bit clearer for the reader that you are generating a temporary file. See: https://docs.python.org/3/library/tempfile.html#examples
6. requests_retry_session() takes a session argument to allow reusing an existing requests.Session(), but a) you never use that argument and b) it doesn't make much sense. As a reader, I would expect a function like this to create a new session every time. If reconfiguring an existing session is part of the scope of that function, then it should indicate that in the name somehow.
7. Also about the sessions, it's a good practice to install a hook on it so that it automatically calls raise_for_status() after every request. That way you don't have to remember to do that manually after every invocation. The syntax may look a bit weird but it's definitely worth it: https://stackoverflow.com/questions/45470226/requests-always-call-raise-for-status
8. Usage of detect_filename() is a bit weird. I would expect a method like that to return the filename instead of updating the filename attribute and not giving anything back.

Bigger stuff:

1. Avoid making calls in function defaults, like you do in __init__. The call is only performed once at method definition time and stored there forever. While, in this case, your cwd is always the same because you're not changing your current directory anywhere else, it's an antipattern to do things like this in Python. It looks weird and you also may get unexpected results if you ever add a chdir somewhere, because the original result to getcwd() will still be the function default. Instead, you should change the dest to None in the method definition and then add an if dest is None: dest = os.getcwd() inside it.
2. Take a look at the pathlib module in the standard library. It can help you simplify most of your file management operations involving os and os.path calls. It is also more robust because it's platform independent. See: https://docs.python.org/3/library/pathlib.html
3. Some of the methods in the class aren't actually related to the class at all. get_valid_filename, fix_existing_filename and requests_retry_session never use self, so it doesn't make a lot of sense for them to be inside the class. Instead you should extract those methods and make them functions. If you really want them to be in a class, use @staticmethod on them so that it's clear that they don't interact with the class or its attributes, but I would recommend the first option.
4. It would be a good idea to store your requests.Session as an attribute, so that you don't have to recreate it every time you invoke download(). The whole point of having a session is to be able to reuse it to take advantage of it saving cookies and keeping connections open.
5. In download(), you set file_size as a new attribute, but that doesn't make a lot of sense. Do you need that to be an attribute? Is it a property of your object? Will you ever need to use it outside the current method? If the answer to all of those are "no", then keep it as a local variable instead.

Nice things:

1. Good separation of your logic across multiple well-defined methods.
2. Informative docstrings, people tend to skip those quite often.
3. tqdm! It's an awesome library and you make the most of it by properly specifying things like units and scaling.
4. Some exception handling is definitely better than nothing. Totally not a showoff, but something important to keep in mind :)
5. Overall it's good code! Don't be discouraged by the number of comments here. You did submit it to this community, so I was nitpicky on purpose, but this code is better than most of what I read at work every day :)