banquise/sujet_ji-reseau
8
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

WIP: demo-server-code-testing #2

Closed
thoms wants to merge 5 commits from demo-server-code-testing into master
pull from: demo-server-code-testing
merge into: banquise:master
Conversation 6 Commits 5 Files changed 3 +290
thoms commented 2025-10-10 14:31:39 +02:00 (Migrated from git.la-banquise.fr)
Copy link

PR comme ca on peut commencer a review meme si ta pas fini @david.cozariuc

PR comme ca on peut commencer a review meme si ta pas fini @david.cozariuc
arthur.wambst (Migrated from git.la-banquise.fr) reviewed 2025-10-10 14:31:40 +02:00
ClementForget (Migrated from git.la-banquise.fr) reviewed 2025-10-10 14:31:41 +02:00
yanis.kocier (Migrated from git.la-banquise.fr) reviewed 2025-10-10 14:31:41 +02:00
ClementForget commented 2025-10-13 13:22:10 +02:00 (Migrated from git.la-banquise.fr)
Copy link

Hello,

@yanis.kocier ce serait possible que tu fasse un petit texte/ebauche de sujet pour qu'on comprenne bien la direction vers laquelle on va ?
Pour le moment te fait pas chier a render le truc, juste le text suffira :)
( sur une autre branch et pr stp )
je veux bien review, mais si deja c'est pas tres clair c'est embetant

Hello, @yanis.kocier ce serait possible que tu fasse un petit texte/ebauche de sujet pour qu'on comprenne bien la direction vers laquelle on va ? Pour le moment te fait pas chier a render le truc, juste le text suffira :) ( sur une autre branch et pr stp ) je veux bien review, mais si deja c'est pas tres clair c'est embetant
ClementForget (Migrated from git.la-banquise.fr) reviewed 2025-10-13 13:29:05 +02:00
ClementForget (Migrated from git.la-banquise.fr) left a comment
Copy link

Hello,
j'ai plusieurs points qui ne me vont pas ici.
Disclaimer : Je peux parraitre "dur" mais je veux juste faire des retours utiles
Ce que je vais dire ici sont mes points globaux, pour le reste cf le reste de la review

But

Pour commencer je me pose la question de ce qu'on veux vraiment leurs montrer.
Dans ce que je vois, on veux leurs faire faire des socket mais en meme temps il a y a pas mal de bordel qui sert pas a grand choses selon moi.
Maintenant @yanis.kocier est ce que le but etait de les faire aussi coder dans connexion.py ?

Forme

Sur la forme aussi j'ai des retours.
Ya bcp de choses qui ressemble a du vibe codding @yanis.kocier
Donc soit on fait un truc full en Fr (ce dont je suis pas fan mais je peux comrendre) soit en anglais, mais pas les deux.
Il faudrais aussi raccroucir les noms de fonctions.

Pour conclure ya encore du taff, j'en discutais avec Arthur, si jamais il est pas pret on fera le sujet python pour la prochaine JI. Donc vaux mieux faire un truc propre, et le proposer plus tard.

@david.cozariuc je sais qu'une bonne partie de ce code n'est pas le tiens, donc tkt :)

Hello, j'ai plusieurs points qui ne me vont pas ici. **Disclaimer : Je peux parraitre "dur" mais je veux juste faire des retours utiles** Ce que je vais dire ici sont mes points globaux, pour le reste cf le reste de la review ## But Pour commencer je me pose la question de ce qu'on veux vraiment leurs montrer. Dans ce que je vois, on veux leurs faire faire des socket mais en meme temps il a y a pas mal de bordel qui sert pas a grand choses selon moi. Maintenant @yanis.kocier est ce que le but etait de les faire aussi coder dans connexion.py ? ## Forme Sur la forme aussi j'ai des retours. Ya bcp de choses qui ressemble a du vibe codding @yanis.kocier Donc soit on fait un truc full en Fr (ce dont je suis pas fan mais je peux comrendre) soit en anglais, mais pas les deux. Il faudrais aussi raccroucir les noms de fonctions. Pour conclure ya encore du taff, j'en discutais avec Arthur, si jamais il est pas pret on fera le sujet python pour la prochaine JI. Donc vaux mieux faire un truc propre, et le proposer plus tard. @david.cozariuc je sais qu'une bonne partie de ce code n'est pas le tiens, donc tkt :)
src/demo_test/client.py
@ -0,0 +33,4 @@
# The client handles each state
##################################
try:
ClementForget (Migrated from git.la-banquise.fr) commented 2025-10-13 13:29:06 +02:00
Copy link

Le try/except KeyboardInterrupt alourdit le code sans réelle valeur ajoutée ici. À retirer je pense.

Le `try/except KeyboardInterrupt` alourdit le code sans réelle valeur ajoutée ici. À retirer je pense.
ClementForget (Migrated from git.la-banquise.fr) commented 2025-10-15 19:22:04 +02:00
Copy link

Je l'ai rajouté pour arrêter le script de façon 'clean' (arrêter la connexion socket).
Sans ça, le socket reste ouvert et tu doit kill le process.

Je l'ai rajouté pour arrêter le script de façon 'clean' (arrêter la connexion socket). Sans ça, le socket reste ouvert et tu doit kill le process.
src/demo_test/client.py
@ -0,0 +34,4 @@
##################################
try:
while etat != "quit":
ClementForget (Migrated from git.la-banquise.fr) commented 2025-10-13 13:31:52 +02:00
Copy link

On est sur de faire la gestion de l'etat avec un str ?
Je veux bien l'avie de @arthur.wambst et @yanis.kocier
Mais selon moi un bon vieux int serait mieux

On est sur de faire la gestion de l'etat avec un str ? Je veux bien l'avie de @arthur.wambst et @yanis.kocier Mais selon moi un bon vieux int serait mieux
src/demo_test/client_modules/connexion.py
@ -0,0 +1,59 @@
"""
ClementForget (Migrated from git.la-banquise.fr) commented 2025-10-13 13:42:14 +02:00
Copy link

J'ai du mal a comprendr l'interet de ce fichier.
L'objectif est de leur apprendre des choses, ici on invisibilse totalement la partie socket.
Il faut bien sur leurs donner des fonctions prefaites, mais pour moi la classe n'a rien a faire ici (on fait pas un tp de POO).
Un fichier separer pourquoi pas, mais je pense qu'ils aurons deja assez de mal avec le code pour en plus leurs ajouter la bonne gestion de l'env i3.
Je veux bien aussi l'avis de @arthur.wambst et @yanis.kocier sur ce sujet

J'ai du mal a comprendr l'interet de ce fichier. L'objectif est de leur apprendre des choses, ici on invisibilse totalement la partie socket. Il faut bien sur leurs donner des fonctions prefaites, mais pour moi la classe n'a rien a faire ici (on fait pas un tp de POO). Un fichier separer pourquoi pas, mais je pense qu'ils aurons deja assez de mal avec le code pour en plus leurs ajouter la bonne gestion de l'env i3. Je veux bien aussi l'avis de @arthur.wambst et @yanis.kocier sur ce sujet
ClementForget (Migrated from git.la-banquise.fr) commented 2025-10-16 06:38:57 +02:00
Copy link

Le connexion.py c'est simplement le module qui permet de simplifier la connexion au socket.

Ça dépends de comment est fait le TP mais je trouve que des lycéens qui n'ont jamais fait du python vont avoir du mal à comprendre le code du socket.

Mais oui après discussion ce module simplifie beaucoup le choses et ont ne va pas l'utiliser.

Le connexion.py c'est simplement le module qui permet de simplifier la connexion au socket. Ça dépends de comment est fait le TP mais je trouve que des lycéens qui n'ont jamais fait du python vont avoir du mal à comprendre le code du socket. Mais oui après discussion ce module simplifie beaucoup le choses et ont ne va pas l'utiliser.
src/demo_test/server.py
@ -0,0 +48,4 @@
def handle_client(self,clientfd,addr:str) -> None:
"""
Magic
ClementForget (Migrated from git.la-banquise.fr) commented 2025-10-13 13:44:40 +02:00
Copy link

Autant ne pas mettre de comment dans ce cas la ..

Autant ne pas mettre de comment dans ce cas la ..
src/demo_test/server.py
@ -0,0 +74,4 @@
clientfd.send(game_menu.print_menu().encode())
choice = clientfd.recv(1024).decode().strip()
if choice == "1":
ClementForget (Migrated from git.la-banquise.fr) commented 2025-10-13 13:38:47 +02:00
Copy link

Les menus et le commentaire d’en-tête ne correspondent pas à client.py. À mettre à jour svp.

Les menus et le commentaire d’en-tête ne correspondent pas à client.py. À mettre à jour svp.
ClementForget (Migrated from git.la-banquise.fr) commented 2025-10-16 06:33:34 +02:00
Copy link

La partie server.py faut pas la prendre en compte.
Je l'ai fait avant de voir le code de yanis et d'avoir une très bonne idée du projet.

La partie server.py faut pas la prendre en compte. Je l'ai fait avant de voir le code de yanis et d'avoir une très bonne idée du projet.
src/demo_test/server.py
@ -0,0 +172,4 @@
Welcome ! \n
Choose an action :\n
\n
[1] - Profile
ClementForget (Migrated from git.la-banquise.fr) commented 2025-10-13 13:45:28 +02:00
Copy link

Pareil, pas les bon noms

Pareil, pas les bon noms
david.cozariuc commented 2025-10-16 06:47:43 +02:00 (Migrated from git.la-banquise.fr)
Copy link

@ClementForget

Merci pour le review, tu as été très clair sur ce qu'il n'y allait pas.

C'était plus l'idée d'utiliser des modules pour le client que je voulais mettre en avant dans ce PR.

Je n'avais d'ailleurs pas utilisé de vibe coding. Les type hints, docstrings, comments: je les rajoute par habitude.

Au final on utilisera la branche #3 et pas celle ci.

@ClementForget Merci pour le review, tu as été très clair sur ce qu'il n'y allait pas. C'était plus l'idée d'utiliser des modules pour le client que je voulais mettre en avant dans ce PR. Je n'avais d'ailleurs pas utilisé de vibe coding. Les type hints, docstrings, comments: je les rajoute par habitude. Au final on utilisera la branche #3 et pas celle ci.

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
arthur.wambst
ClementForget
yanis.kocier
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
banquise/sujet_ji-reseau!2
No description provided.