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

[Doctype] Refactoring des modèles dérivés de Document #696

Open
edas opened this issue Aug 2, 2019 · 4 comments
Open

[Doctype] Refactoring des modèles dérivés de Document #696

edas opened this issue Aug 2, 2019 · 4 comments

Comments

@edas
Copy link
Contributor

edas commented Aug 2, 2019

Nous avons une suite de modèles dérivés de Document.

Ces modèles me posent un problème sur l'architecture. On utilise une classe avec des méthodes statiques qui gardent un état global à toutes les applications. C'est peu évolutif et on touche déjà des limites en créant des méthodes comme copyWithClient() pour contourner le singleton.

Voici ce que je propose :

[ ] Retoucher Document pour transformer toutes les méthodes statiques en méthodes d'instance
[ ] Retirer l'export par défaut de Document pour le transformer en un const document = new Document() et exporter cette instance (toujours la même donc)

En soi c'est rapide à faire et normalement ça devrait être transparent pour tout ce qui utiliser Document. Celui qui veut accéder à la classe maitresse pour en créer une nouvelle instance peut facilement faire ensuite appel à la propriété constructor.

Le problème c'est qu'on a plein de modèles qui dérivent de Document via un simple extends et la même architecture de singleton. Ça veut dire faire la même chose sur tous ces modèles dérivés.
Ces modèles dérivés partagent aussi l'état enregistré dans Document (les index, le client) donc ça veut dire un peu plus de travail pour y faire de la composition. Ca ne devrait toutefois pas être très complexe/long.

L'enjeu c'est qu'il y a beaucoup de classes à modifier ainsi :

Dans Cozy-Libs
[ ] BankTransaction (Transaction)
[ ] File (CozyFile)
[ ] BalanceHistory
[ ] BankAccountStats
[ ] Contact
[ ] BankAccount
[ ] AdministrativeProcedure
[ ] Account
[ ] Group
[ ] Permission
[ ] Application
[ ] examples et docs

Dans Cozy-Banks
[ ] AppSuggestion
[ ] Trigger
[ ] Settings

En comptant 1/2 journée pour Document, 1/2 journée pour la doc, 1 jour de test et revue, puis 5 classes dérivées par jour, ça fait quand même 5 jours de travail.

@y-lohse
Copy link
Contributor

y-lohse commented Aug 2, 2019

J'ai poussé ce que j'avais commencé a faire (sans le constructor, mais approche similaire) sur c2a2505, au cas ou. J'ai me suis interrompu après avoir pas mal lutté contre des tests qui cassaient.

Comme évoqué ici il y aussi les propriétés statiques des différentes sous-classes à ne pas oublier.

Et enfin, ca sera un breaking change. Avant on pouvait faire:

Document.registerClient(clientInstance)
BankHistory.doSomethingWithClient()

et ca fonctionnait (parce que clientInstance était partagé entre les classes), mais une fois ces changements faits on ne pourra plus (je pense quand meme que c'est un bon changement, mais effectivement il faut prendre le temps de le faire correctement).

@ptbrowne
Copy link
Contributor

ptbrowne commented Aug 2, 2019

As an experiment, I tried to see if adding the cozyClient parameter to method statically would work. https://astexplorer.net/#/gist/c78b1c9f816b0237c2a2a4875c82a33c/a8e66318ba734a8fa59c70ffd58b5986b149395f

@edas
Copy link
Contributor Author

edas commented Aug 5, 2019

@y-lohse C'est faisable via un peu de composition, sans casser l'usage existant. Ca ne me parait pas bloquant

@ptbrowne Ca tourne dans ma tête mais je ne suis toujours pas fana de l'idée de méthodes statiques auxquelles on passe une instance cozy-client à chaque appel. Ca te gêne d'avoir une instance pour garder ce genre de choses dans l'état ?

@ptbrowne
Copy link
Contributor

ptbrowne commented Aug 5, 2019

Ca te gêne d'avoir une instance pour garder ce genre de choses dans l'état ?

Non pas vraiment, mais je vois pas quel est l'avantage ? Pour moi l'idée était de simplifier/expliciter les choses en ayant que des méthodes qui ne servent pas du this.

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

No branches or pull requests

3 participants