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

Réforme dynamique : Corrige le problème de non création de conditions lorsqu'un profil est présent deux fois #194

Merged
merged 5 commits into from
Dec 6, 2023

Conversation

Allan-CodeWorks
Copy link
Contributor

Description

Si un fichier yaml présente deux profils de même type, la deuxième condition écrase la première.

@Allan-CodeWorks Allan-CodeWorks requested a review from a team November 29, 2023 10:19
@Allan-CodeWorks Allan-CodeWorks self-assigned this Nov 29, 2023
@Allan-CodeWorks Allan-CodeWorks requested review from baptou12 and a team November 30, 2023 15:44
@Allan-CodeWorks Allan-CodeWorks force-pushed the Fix_reforme_double_profil branch 3 times, most recently from 8bc8c85 to 3fd88e4 Compare December 5, 2023 13:19
@Allan-CodeWorks Allan-CodeWorks requested review from baptou12 and a team December 5, 2023 13:20
Copy link
Contributor

@baptou12 baptou12 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 va, quelques suggestions sur le code, rien de primordiale, overall je trouve ça complexe, mais en même temps c'est un sujet compliqué

def condition_already_exists_in_node(profil_condition, conditions_in_node_data) -> bool:
for condition_type in profil_condition.keys():
if condition_type in conditions_in_node_data:
conditions_with_operator_fields = ['age', 'quotient_familial', 'situation_handicap']
Copy link
Contributor

Choose a reason for hiding this comment

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

cette définition on pourrait la remonté avant le for, ça rendrait un peu mieux j'ai l'impression, t'en pense quoi ?


def add_profil_with_conditions(data: dict, profil: dict):
data['profils'][profil['type']].update(conditions_to_node_data(profil['conditions']))
def condition_already_exists_in_node(profil_condition, conditions_in_node_data) -> bool:
for condition_type in profil_condition.keys():
Copy link
Contributor

Choose a reason for hiding this comment

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

Pas obligé, mais pour facilité la lecture ce que je fais parfois c'est réduire le nombre de branche possibles le plus rapidement possible (1). Par exemple là jusqu'à la toute fin de la méthode on doit gardé en tête qu'est-ce qui se passe si profil_condition.keys() est vide. On pourrait mettre un early return pour enlever ce cas des possible. Ça permetrait en plus ligne 118 de return directement operator in conditions_in_node_data[condition_type]

(1) oui ça fait deux fois possible :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je ne vois pas bien ce que tu veux dire :

  • si profil_condition.keys() est vide, on ne rentre pas dans le for
  • Je peux déjà return comme tu le dis en ligne 118 (et merci, je l'ai fait)

Copy link
Contributor

@baptou12 baptou12 Dec 6, 2023

Choose a reason for hiding this comment

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

Justement, mais là on doit garder en tête que si c'est vide faudrait regarder après le for
Alors que si on fait avant le for

if len(profil_condition) == 0:
    return False

On est sûr à 100% qu'on rentre dans le for, et on ecarte à 100% le cas profile_condition est vide.

Ça change pas grand chose et c'est même plus long, mais ça facilite la lecture (enfin je trouve)

Copy link
Contributor

Choose a reason for hiding this comment

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

Encore une fois pas obligé, voici un blog post qui parle entre autre de cet argument.

https://freedium.cfd/https://betterprogramming.pub/are-early-returns-any-good-eed4b4d03866

The last argument that I'm going to make for using early returns is peace of mind for the programmer. By using early returns, you get invalid cases out of the way first (bouncer pattern), put a blank line in there next, and then you can focus on the "real" body of the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

D'accord, je comprends!
C'est un article intéressant et je vois maintenant l'intérêt pour le cas ici, je trouve effectivement que c'est pas plus mal de rendre ce code un peu plus facile à lire et je pense que c'est le cas avec ce "early return"
Merci!

…entiques pour les aides avec profiles identiques
@Allan-CodeWorks Allan-CodeWorks merged commit d41a7de into master Dec 6, 2023
9 checks passed
@Allan-CodeWorks Allan-CodeWorks deleted the Fix_reforme_double_profil branch December 6, 2023 14:36
@guillett guillett added this to the Passé milestone Mar 22, 2024
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.

3 participants