Descargador en Python

Question

Escribí este código en python y funciona bien para mí, pero sé que el código no está optimizado y es necesario realizar una gran cantidad de refactorización. Así que necesito revisar cómo se puede mejorar este código. Empecé a escribir esto cuando la biblioteca WGET no me funcionaba y quería un guión ligero para mis otros proyectos. También estoy pensando en reemplazar la biblioteca de solicitudes con aiohttp. Siendo un principiante en esto, espero sus revisiones.

Gracias.

Requisitos : - tqdm , solicitudes

Implementación: - 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'

Uso:-

# 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

Primero, un par de cosas de estilo / pelusa:

1. Está importando HTTPError e InvalidSchema de las solicitudes.excepciones, pero no las está utilizando.
2. Sea coherente con su sangría. 4 espacios es el número recomendado por PEP8 y está bien si no quiere seguir eso, pero trate de no mezclar 2 y 4 espacios de sangría en el mismo proyecto, como lo está haciendo adentrorequests_retry_session()
3. Intente usar formato de cadena en lugar de concatenar con +. Esto le ahorra la molestia de convertir manualmente los valores a str(como lo hace con uuid download()) y también es más fácil de leer. Eche un vistazo a f-strings si está en Python 3.6+ (que debería):https://realpython.com/python-f-strings/
4. No se compare Nonecon ==. Usar la ispalabra clave es la forma más idiomática de hacerlo. La primera línea detect_filename()se puede reescribir como if self.filename is not None. Ver:https://stackoverflow.com/questions/14247373/python-none-comparison-should-i-use-is-or
5. Como regla general, el código comentado es algo que no necesitamos, por lo que es mejor que lo eliminemos por completo. Si alguna vez necesita esa línea, siempre puede obtenerla de su historial de git. Porque estás usando git, ¿verdad? ¿¿DERECHO??

Cosas menores y delicadas:

1. La última parte download()usa una excepción básica, que suele ser una mala idea porque detecta algunas excepciones que probablemente no quieras detectar. Ver:https://stackoverflow.com/questions/54948548/what-is-wrong-with-using-a-bare-except
2. Su docstring para fix_existing_filename()dice que verifica si el nombre de archivo ya existe, pero en realidad no lo hace.
3. En download(), no necesita abrir el archivo como lectura-escritura si no tiene la intención de leerlo. Configurar el modo de apertura en solo wbdeja más claro para el lector que solo tiene la intención de escribir en ese archivo.
4. En check_if_dir_existque no es necesario la ifdeclaración, porque se puede pasar exist_ok=Truea os.makedirsy que creará automáticamente el directorio sólo si no existe. De hecho, me desharía de este método por completo porque puede hacer todo en una línea.
5. En lugar de generar un nombre de archivo temporal usted mismo, eche un vistazo al tempfilemódulo en la biblioteca estándar. No solo resuelve el mismo problema con el que lo hizo uuid4, sino que también es un poco más claro para el lector que está generando un archivo temporal. Ver:https://docs.python.org/3/library/tempfile.html#examples
6. requests_retry_session()toma un sessionargumento para permitir reutilizar uno existente requests.Session(), pero a) nunca usas ese argumento yb) no tiene mucho sentido. Como lector, esperaría que una función como esta cree una nueva sesión cada vez. Si reconfigurar una sesión existente es parte del alcance de esa función, entonces debería indicarlo en el nombre de alguna manera.
7. También sobre las sesiones, es una buena práctica instalar un gancho en él para que llame automáticamente raise_for_status()después de cada solicitud. De esa forma, no es necesario que recuerde hacerlo manualmente después de cada invocación. La sintaxis puede parecer un poco extraña, pero definitivamente vale la pena:https://stackoverflow.com/questions/45470226/requests-always-call-raise-for-status
8. El uso de detect_filename()es un poco extraño. Esperaría que un método como ese devuelva el nombre del archivo en lugar de actualizar el filenameatributo y no devolver nada.

Cosas más grandes:

1. Evite realizar llamadas con funciones predeterminadas, como lo hace en __init__. La llamada solo se realiza una vez en el momento de la definición del método y se almacena allí para siempre. Si bien, en este caso, su cwdsiempre es el mismo porque no está cambiando su directorio actual en ningún otro lugar, es un antipatrón hacer cosas como esta en Python. Parece extraño y también puede obtener resultados inesperados si alguna vez agrega un chdirlugar, porque el resultado original getcwd()seguirá siendo la función predeterminada. En su lugar, debe cambiar desta Noneen la definición del método y luego agregar un if dest is None: dest = os.getcwd()dentro de él.
2. Eche un vistazo al pathlibmódulo en la biblioteca estándar. Puede ayudar a simplificar la mayor parte de sus operaciones de gestión de archivos que implican osy os.pathlas llamadas. También es más robusto porque es independiente de la plataforma. Ver:https://docs.python.org/3/library/pathlib.html
3. Algunos de los métodos de la clase no están realmente relacionados con la clase. get_valid_filename, fix_existing_filenameY requests_retry_sessionnunca uso self, por lo que no tiene mucho sentido para ellos estar dentro de la clase. En su lugar, debe extraer esos métodos y convertirlos en funciones. Si realmente desea que estén en una clase, utilícelos @staticmethodpara que quede claro que no interactúan con la clase o sus atributos, pero recomendaría la primera opción.
4. Sería una buena idea almacenar su requests.Sessioncomo atributo, para que no tenga que volver a crearlo cada vez que lo invoque download(). El objetivo de tener una sesión es poder reutilizarla para aprovecharla guardando cookies y manteniendo abiertas las conexiones.
5. En download(), se establece file_sizecomo un atributo nuevo, pero eso no tiene mucho sentido. ¿Necesitas que sea un atributo? ¿Es una propiedad de su objeto? ¿Alguna vez necesitará usarlo fuera del método actual? Si la respuesta a todas ellas es "no", entonces manténgala como una variable local.

Cosas bonitas:

1. Buena separación de su lógica a través de múltiples métodos bien definidos.
2. Cadenas de documentación informativas, las personas tienden a omitirlas con bastante frecuencia.
3. tqdm! Es una biblioteca impresionante y la aprovechas al especificar correctamente cosas como unidades y escalas.
4. Cierto manejo de excepciones es definitivamente mejor que nada. Totalmente no un alarde, pero algo importante a tener en cuenta :)
5. En general, es un buen código. No se desanime por la cantidad de comentarios aquí. Lo enviaste a esta comunidad, así que fui quisquilloso a propósito, pero este código es mejor que la mayoría de lo que leo en el trabajo todos los días :)