-
Notifications
You must be signed in to change notification settings - Fork 7
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
Backend renommage #3386
Backend renommage #3386
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go pour merge 👍
Super pour le renommage de l'existant conforme aux conventions.
Je me permets de rappeler néanmoins que ça fait beaucoup de schéma de nouvelles tables ajoutées sans utilisation. Idem pour certain services comme axe.service
ou fiche.service
qui contiennent des méthodes non utilisées.
Je comprends l'intention derrière la démarche "ça servira pour plus tard" mais c'est une fausse bonne pratique, d'autant plus dans cette volumétrie. Pour une personne qui n'a pas écrit ce code, il faut tout d'abord le trouver, savoir ensuite si le code répond à son besoin, savoir si ce code non utilisé et non testé peut finalement être utilisé ou non. Je pense que ca soulève potentiellement pus de questions que ça n'en résout.
Un commentaire particulier sur la taille de la PR également. Parfois on n'a pas le choix pour des grosses refactos, mais en règle générale plutôt essayer de découper les gros chantiers en petit lot de plus petites PRs : plus simple à review, moins risqué à livrer en prod.
Go pour merge là pour avancer et ne pas perdre tout ce travail effectué, mais à l'avenir, faisons des refactos incrémentales, avec du code ajouté qui est forcément utilisé au sein de la PR.
Merci 🙏
0e71462
to
6e19050
Compare
No description provided.