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

[FEAT] RTTTL support #1

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

[FEAT] RTTTL support #1

wants to merge 2 commits into from

Conversation

Rijaja
Copy link

@Rijaja Rijaja commented Jun 8, 2024

J'ai ajouté la lecture de RTTTL que j'avais fait durant l'atelier il y a un mois

J'ai plein de questions de conceptions et je suis ouvert si vous voulez que je change un truc:

  • J'utilise des types explicites presque partout, mais ce n'est pas fait dans le reste du code. Je les enlève ?
  • J'ai mis une constante pour les notes dans la section buzzer. Vous voulez que je la mette en haut avec les autres constantes ? Vous voulez que je la contienne dans la méthode qui l'utilise ?
  • J'ai fait une méthode qui lit les RTTTL, une qui les joue, puis un wrapper qui appelle les deux. J'ai fait ça pour pouvoir ajouter d'autres sources plus tard mais je peux fusionner les méthodes si vous voulez.

Si vous avez d'autres demandes hésitez pas. Par exemple mettre au moins read_rtttl dans un fichier à part car elle est longue.

elio.py Show resolved Hide resolved
@MaximeMauduit
Copy link
Collaborator

  • Pour les types explicites je pense que c'est une bonne idée à garder.
  • Comme ta constante est utilisé seulemnet dans une méthode et je doute qu'on la réutilise ailleur j'aurais tendance à dire qu'on peux la mettre dans la méthode.
  • L'idée d'avoir le wrapper qui appelle les méthode est sympa ça permet de mieux comprendre chaque fonction vue qu'elle sont décomposé.

@Rijaja
Copy link
Author

Rijaja commented Jun 13, 2024

Cause probable du bug: adafruit/circuitpython#5938

Je vais tenter de trouver une solution, ou abandonner l'indication de la note invalide

@Rijaja
Copy link
Author

Rijaja commented Jun 13, 2024

J'ai fait un truc à l'ancienne avec ' but got "' + note_string + '" instead.', ça devrait marcher mais c'est moins propre qu'un f string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants