-
-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🔥 Remove twitter from the rss plugin & rework a little bit how setup works internally #242
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review rapide, semble correct. À tester de manière extensive.
Est-ce que tu pourrais également fix l'erreur PyLint ?
Elle semble dûe au fait que le setup.py gère de façon incorrecte l'acceptation/le refus (un coup sur deux il vérifie que la réponse est un refus, un coup sur deux que c'est une acceptation, elle devrait faire les deux)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je ne pense pas que ce soit la bonne manière de faire, en règle générale le "non" doit primer sur le "oui".
Donc dans l'idéal il faudrait considérer les deux entrées et envoyer un message comme "Nous n'avons pas compris, veuillez entrer 'oui' ou 'non'" quand l'utilisateur rentre "banane".
Ou, au pire, tout considérer comme "non" à moins que ce soit dans la liste de valeurs "oui"
Comme indiqué par l'input, la valeur sélectionnée par défaut est l'activation de la boucle RSS |
3747924
to
fa1e7ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ça me paraît globalement bon, je vais tester les modifications d'Aeris de mon côté.
C'est moi qui ait push eh, je peux pas review moi-même
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fa1e7ed
to
f3511ef
Compare
f3511ef
to
7a6156d
Compare
Closes #235.