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

Organisation des fichiers #18

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

Baduit
Copy link

@Baduit Baduit commented Oct 20, 2024

Pour l'instant vu que le projet n'est pas très gros, avoir toutes les sources au même niveau avec les assets ça passe mais si il y a des contributions ça peut vite devenir n'importe quoi.

Dans cette PR je réorganise tout ça:

  • Toutes les sources sont dans src/croix_pharmacie
  • Les fichiers génériques sont à la racine de src/croix_pharmacie
  • Les scripts qu'on peut exécuter sont dans src/croix_pharmacie/mains
  • Les assets sont dans src/croix_pharmacie/assets

J'ai aussi rajouté un pyproject.toml, ça permet de pouvoir faire un pip install . après avoir clone ou sinon un pip install pip install git+https://github.com/MathisHammel/CroixPharmacie installera toute les dépendances tout seul.

Dans ce pyproject.toml j'ai ajouté des "project.scripts" pour chaque scripts, ce qui fait qu'après l'installation on peut faire au choix un flappyBird ou bien un python ./src/croix_pharmacie/mains/flappyBird.py ou encore un rye run flappyBird si on utilise rye.

Pour travailler sur le projet, je recommande d'utiliser rye qui est vraiment super pratique, Il suffit de faire des rye sync et des rye run pour tester des trucs. Mais sinon juste faire un pip install --editable . une fois permet ensuite toutes les modifications seront prises en compte.

Aussi pour éviter des soucis avec des paths relatifs, j'ai fait une fonction get_asset_path qui permet de d'avoir le path vers un asset et ça marche même si le package a été installé.

J'ai aussi fait en sorte qu'il n'y ait plus besoin d'installer doom à la main et maintenant c'est installé avec les autres dépendances.

Au passage j'ai aussi fait de petites correction dans certaines classes où au lieu d'utiliser self ça utilisait une variable locale au fichier. En plus j'ai aussi faits des petites corrections que mon linter a souligné mais ça change vraiment pas grand chose (surtout des x == False qui sont remplaces par des not x ou x is False.

Je suis consciente que ça fait beaucoup de changements et ça peut sembler un peu plus compliquer à utiliser mais ça permet d'avoir un truc bien organisé et qui utilise la une manière standard d'installé un package.

Si c'est pas clair hésitez pas à poser des questions (surtout que bon là il commence à se faire tard donc je divague peut être un peu) et aussi hésitez pas faire des retours quittes à ce que je doive changer ma PR.

PS: j'ai beaucoup aimé la vidéo

Copy link

@Athroniaeth Athroniaeth left a comment

Choose a reason for hiding this comment

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

J'ai regardé en diagonale, c'est très propre et c'est clairement un plus pour l'organisation du projet.

src/croix_pharmacie/asset_helper.py Show resolved Hide resolved
src/croix_pharmacie/mains/videoplayer.py Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@raphaeldenni
Copy link

Hello, très bonne initiative, je voulais faire de même, mais j'ai été pris de cours on dirait x)

Je voulais déjà demander pourquoi le dossier des scripts se nomme "mains"? Je ne trouve pas ça très clair, on a l'impression que c'est la partie de code la plus importante, alors que c'est pharmacontroller.py le point central. Peut-être utiliser "scripts" justement ou encore "demo" puisque ça présente le projet aussi.

Ensuite, je pense qu'il serait bien d'ajouter des consignes dans le README, ou autre part, pour savoir où placer les prochains scripts, histoire d'éviter dans les PR à devoir écrire "SVP mettez le script là où il faut" x) -> ça je peux le faire, mais je préfère avoir l'avis des gens avant

@sylvqin
Copy link

sylvqin commented Oct 21, 2024

je viens juste dire merci pour le compliment sur la vidéo <3 et je laisse Mathis voir les PR ahah mais ça fait plaisir de voir le repo continuer de vivre

@Baduit
Copy link
Author

Baduit commented Oct 21, 2024

Hello, très bonne initiative, je voulais faire de même, mais j'ai été pris de cours on dirait x)

Je voulais déjà demander pourquoi le dossier des scripts se nomme "mains"? Je ne trouve pas ça très clair, on a l'impression que c'est la partie de code la plus importante, alors que c'est pharmacontroller.py le point central. Peut-être utiliser "scripts" justement ou encore "demo" puisque ça présente le projet aussi.

Ensuite, je pense qu'il serait bien d'ajouter des consignes dans le README, ou autre part, pour savoir où placer les prochains scripts, histoire d'éviter dans les PR à devoir écrire "SVP mettez le script là où il faut" x) -> ça je peux le faire, mais je préfère avoir l'avis des gens avant

Il se nomme mains car dedans il y a les fichiers avec des fonctions main et que j'étais pas inspirée du tout 😁
Je vais changer ça en scripts ça m'a l'air bien bien, à moins que quelqu'un ait une meilleur idée

Les consignes dans le readme c'est une bonne idée, ça permettra que ça soit encore plus facile de contribuer et à utiliser. Si ça te dérange pas de faire la doc fait toi plaisir, c'est clairement pas la partie que je préfère 😅 Je pense que ça peut être fait dans une autre PR à la suite de celle ci (après ça dépend de comment Mathis préfère organiser ça)

- Organize all the sources as a package named croix_pharmacie
- Use a pyproject.toml to have a standard way to install the package
- Use rye as a build system (not mandatory to use)
- Add a way to access the assets even after installation and without relying on hard coded paths
- Add a main function in all scripts that only had a if main
- Add "project.scripts" in the pyproject.toml for each main to make them easy to launch
- little fixes for some classes were a variable declared at the file level was used instead of self
- Fix relative link to module files
- Update installation process
No need to install it manually anymore
@Baduit
Copy link
Author

Baduit commented Oct 22, 2024

J'ai mis à jour ma PR pour renommer le dossir mains en scripts

J'en ai aussi profité pour pour la dépendance vers cydoomgeneric d'utiliser le repos qui était de base dans le readme https://github.com/wojciech-graj/cydoomgeneric au lieu de mon fork https://github.com/Baduit/cydoomgeneric vu que ma PR a été mergée wojciech-graj/cydoomgeneric#4

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.

4 participants