Skip to content
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

Merged
merged 5 commits into from
Nov 2, 2023

Conversation

ascpial
Copy link
Contributor

@ascpial ascpial commented Aug 30, 2023

Closes #235.

@ascpial ascpial changed the title 🔥 >emove twitter from the rss plugin 🔥 Remove twitter from the rss plugin Aug 30, 2023
@ascpial ascpial requested a review from a team August 30, 2023 16:58
@ascpial ascpial added the plugin label Aug 30, 2023
Copy link
Collaborator

@Aeris1One Aeris1One left a 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)

@ascpial ascpial requested a review from Aeris1One August 30, 2023 17:53
Copy link
Collaborator

@Aeris1One Aeris1One left a 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"

@ascpial
Copy link
Contributor Author

ascpial commented Aug 31, 2023

Comme indiqué par l'input, la valeur sélectionnée par défaut est l'activation de la boucle RSS (Y/n). Il me semble donc approprié de vérifier que la valeur n'est pas négative.

@Aeris1One Aeris1One requested a review from a team August 31, 2023 09:46
Aeris1One
Aeris1One previously approved these changes Aug 31, 2023
@Aeris1One Aeris1One requested a review from a team August 31, 2023 09:47
@Aeris1One Aeris1One changed the title 🔥 Remove twitter from the rss plugin 🔥 Remove twitter from the rss plugin & rework a little bit how setup works internaly Aug 31, 2023
@Aeris1One Aeris1One changed the title 🔥 Remove twitter from the rss plugin & rework a little bit how setup works internaly 🔥 Remove twitter from the rss plugin & rework a little bit how setup works internally Aug 31, 2023
Copy link
Contributor Author

@ascpial ascpial left a 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é.

setup.py Show resolved Hide resolved
@Aeris1One Aeris1One dismissed their stale review August 31, 2023 10:02

C'est moi qui ait push eh, je peux pas review moi-même

Aeris1One
Aeris1One previously approved these changes Aug 31, 2023
@Aeris1One Aeris1One dismissed their stale review August 31, 2023 10:03

MAIS GITHUB T'ES BÊTE OU QUOI ?

@Aeris1One Aeris1One requested a review from a team August 31, 2023 10:03
Copy link
Contributor

@ZRunner ZRunner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Aeris1One Aeris1One merged commit bea75dc into beta Nov 2, 2023
2 checks passed
@Aeris1One Aeris1One deleted the thanosed-twitter branch November 2, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable Twitter RSS features
3 participants