-
Notifications
You must be signed in to change notification settings - Fork 0
Review #25
Copy link
Copy link
Open
Description
Salve,
ecco un feedback costruttivo sul progetto:
PROs
- il codice è tipizzato, anche se con qualche piccola imperfezione (es. la funzione
process_elements_listdal tipo sembra che ritorni un int, invece ritorna una tupla di valori) - ottimo README
- avete utilizzato I secrets per definire il TOKEN, strategia non male, ma idealmente andrebbe creato un mock, però immagino non sia immediato da fare (io per eseguire I tests in locale ho semplicemente usato
export TOKEN="123", però avete fatto bene ad usare I secret vi torneranno utili in futuro) - ottimi tests
- code coverage 94% 🚀
CONs
- config.py si poteva gestire in un altro modo, potenzialmente l’utente che vuole usare il vostro progetto non dovrebbe modificare il codice in alcun modo ma al massimo modificare un file di config (un file yaml, json o comunque un file di testo, potenzialmente un file in .gitignore)
- pylint –exit-zero ❌☠️ , aprendo il primo file del progetto, init.py anche dall’IDE stesso noto che ci sono degli “unusued imports”, pylint li avrà sicuramente segnalati ma se mettete –exit-zero permettete a qualsiasi sviluppatore (oltre voi stessi) di “ignorare” gli errori di lint, permettendo di creare dead code (codice inutilizzato) o importando librerie che non verranno mai usate ma che potenzialmente potrebbero aumentare inutilmente la grandezza del software.
So che avete messo quel parametro per non far fallire la pipeline, però il far fallire la pipeline è proprio per forzare ad ogni sviluppatore coinvolto di programmare “bene” evitando di fargli lasciare codice incompleto o scritto male.
Per “disabilitare” alcune regole di pylint si pòu usare il file .pylintrc, oppure si possono disabilitare le singole righe usando un commento simile a “pylint: disable E0401”, al posto di non far fallire mai la pipeline.
Il progetto è molto meglio rispetto a prima, ha una pipeline che funziona (anche se parzialmente ignorata), è pieno di tests ed I commits sono coerenti con I conventional commits.
Ottimo lavoro 💪
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels