Downloader in Python

Question

Ho scritto questo codice in Python e per me funziona bene, ma so che il codice non è ottimizzato e deve essere fatto molto refactoring. Quindi ho bisogno di una revisione su come questo codice può essere migliorato. Ho iniziato a scrivere questo quando la libreria WGET non funzionava per me e volevo uno script leggero per i miei altri progetti. Sto anche pensando di sostituire la libreria delle richieste con aiohttp. Essendo un principiante in questo, attendo con impazienza le tue recensioni.

Grazie.

Requisiti : - tqdm , richieste

Implementazione: - 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'

Utilizzo: -

# 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

Innanzitutto, un paio di cose di stile / linting:

1. Stai importando HTTPError e InvalidSchema da requests.exceptions ma non li stai utilizzando.
2. Sii coerente riguardo al tuo rientro. 4 spazi è il numero consigliato da PEP8 e va bene se non vuoi seguirlo, ma cerca di non mescolare 2 e 4 rientri di spazio nello stesso progetto, come stai facendo all'internorequests_retry_session()
3. Prova a utilizzare la formattazione delle stringhe invece di concatenare con +. Questo ti risparmia la fatica di convertire manualmente i valori in str(come fai con l'uuid in download()) ed è anche più facile da leggere. Dai un'occhiata alle stringhe f se sei su Python 3.6+ (cosa che dovresti):https://realpython.com/python-f-strings/
4. Non confrontare Nonecon ==. Usare la isparola chiave è il modo più idiomatico di farlo. La prima riga detect_filename()può essere riscritta come if self.filename is not None. Vedere:https://stackoverflow.com/questions/14247373/python-none-comparison-should-i-use-is-or
5. Come regola generale, il codice commentato è qualcosa di cui non abbiamo bisogno, quindi potremmo anche eliminarlo completamente. Se hai bisogno di quella linea indietro, puoi sempre ottenerla dalla tua cronologia di git. Perché stai usando git, giusto? DESTRA??

Cose minori e nitide:

1. L'ultimo pezzo di download()usa un nudo tranne, che di solito è una cattiva idea perché cattura alcune eccezioni che probabilmente non vuoi catturare. Vedere:https://stackoverflow.com/questions/54948548/what-is-wrong-with-using-a-bare-except
2. La tua docstring per fix_existing_filename()dice che controlla se il nome del file esiste già, ma in realtà non lo fa.
3. In download(), non è necessario aprire il file in lettura-scrittura se non si intende effettivamente leggere da esso. L'impostazione della modalità di apertura su wbrende più chiaro per il lettore che intendi scrivere solo su quel file.
4. In check_if_dir_existnon hai bisogno ifdell'istruzione, perché puoi passare exist_ok=Truea os.makedirse questo creerà automaticamente la directory solo se non esiste. In effetti, mi sbarazzerei completamente di questo metodo perché puoi fare tutto in una riga.
5. Invece di generare tu stesso un nome di file temporaneo, dai un'occhiata al tempfilemodulo nella libreria standard. Non solo risolve lo stesso problema che hai risolto uuid4, ma è anche un po 'più chiaro per il lettore che stai generando un file temporaneo. Vedere:https://docs.python.org/3/library/tempfile.html#examples
6. requests_retry_session()richiede un sessionargomento per consentire il riutilizzo di un esistente requests.Session(), ma a) non usi mai quell'argomento eb) non ha molto senso. Come lettore, mi aspetto che una funzione come questa crei ogni volta una nuova sessione. Se la riconfigurazione di una sessione esistente fa parte dell'ambito di quella funzione, allora dovrebbe indicarlo in qualche modo nel nome.
7. Anche per quanto riguarda le sessioni, è buona norma installare un hook su di esso in modo che chiami automaticamente raise_for_status()dopo ogni richiesta. In questo modo non devi ricordarti di farlo manualmente dopo ogni chiamata. La sintassi può sembrare un po 'strana ma ne vale sicuramente la pena:https://stackoverflow.com/questions/45470226/requests-always-call-raise-for-status
8. L'utilizzo di detect_filename()è un po 'strano. Mi aspetto che un metodo come questo restituisca il nome del file invece di aggiornare l' filenameattributo e non restituire nulla.

Cose più grandi:

1. Evita di effettuare chiamate nelle impostazioni predefinite delle funzioni, come fai in __init__. La chiamata viene eseguita solo una volta al momento della definizione del metodo e memorizzata lì per sempre. Mentre, in questo caso, il tuo cwdè sempre lo stesso perché non stai cambiando la tua directory corrente da nessun'altra parte, è un antipattern fare cose del genere in Python. Sembra strano e potresti anche ottenere risultati inaspettati se aggiungi un chdirda qualche parte, perché il risultato originale getcwd()sarà ancora quello predefinito della funzione. Invece, dovresti cambiare il desta Nonenella definizione del metodo e quindi aggiungere un if dest is None: dest = os.getcwd()al suo interno.
2. Dai un'occhiata al pathlibmodulo nella libreria standard. Può aiutarti a semplificare la maggior parte delle operazioni di gestione dei file che coinvolgono ose os.pathchiamate. È anche più robusto perché è indipendente dalla piattaforma. Vedere:https://docs.python.org/3/library/pathlib.html
3. Alcuni dei metodi della classe non sono affatto correlati alla classe. get_valid_filename, fix_existing_filenameE requests_retry_sessionnon usare mai self, in modo da non fare un sacco di senso per loro di essere all'interno della classe. Invece dovresti estrarre quei metodi e renderli funzioni. Se vuoi davvero che siano in una classe, usali @staticmethodin modo che sia chiaro che non interagiscono con la classe oi suoi attributi, ma consiglierei la prima opzione.
4. Sarebbe una buona idea memorizzare il tuo requests.Sessioncome attributo, in modo da non doverlo ricreare ogni volta che lo invochi download(). Il punto centrale di una sessione è poterla riutilizzare per trarne vantaggio salvando i cookie e mantenendo aperte le connessioni.
5. In download(), imposti file_sizecome nuovo attributo, ma non ha molto senso. Hai bisogno che sia un attributo? È una proprietà del tuo oggetto? Avrai mai bisogno di usarlo al di fuori del metodo attuale? Se la risposta a tutte queste domande è "no", mantienila invece come variabile locale.

Belle cose:

1. Buona separazione della logica tra più metodi ben definiti.
2. Docstring informativi, le persone tendono a saltarli abbastanza spesso.
3. tqdm! È una libreria fantastica e puoi trarne il massimo specificando correttamente cose come unità e ridimensionamento.
4. Una certa gestione delle eccezioni è decisamente meglio di niente. Non è assolutamente uno spettacolo, ma qualcosa di importante da tenere a mente :)
5. Nel complesso è un buon codice! Non scoraggiarti dal numero di commenti qui. L'hai inviato a questa comunità, quindi sono stato pignolo di proposito, ma questo codice è migliore della maggior parte di quello che leggo al lavoro ogni giorno :)