DGNum/gestioCOF
13
2
Fork 1
Code Issues 77 Pull requests 14 Packages Projects Releases 12 Wiki Activity

L'autocomplétion est isolée et réutilisable par d'autres apps #694

Merged
mpepin merged 0 commits from kerl/autocomplete into master 2020-05-07 16:28:47 +02:00
Conversation 33 Commits - Files changed -
mpepin commented 2019-12-11 22:06:12 +01:00 (Migrated from git.eleves.ens.fr)
Copy link

Idée : on écrit des petites unités de recherche qui cherche à un endroit (genre une table SQL, le LDAP, etc) et on laisse la classe Compose agréger les résultats.

  • On avait déjà une classe ModelSearch
  • J'ajoute une classe LDAPSearch avec une interface similaire
  • Compose fait le taf d'appeler tout le monde et de virer les doublons.

À propos des doublons : je ne suis pas hyper convaincu de ce que j'ai fait mais j'ai pas tellement de meilleure idée, en tout cas pas qui soit aussi simple. Des idées ou on garde ça ?

À voir ensemble (au moins) :

  • Est-ce que le design général est ok ?
  • Est-ce que les docstrings sont claires ?
Idée : on écrit des petites unités de recherche qui cherche à un endroit (genre une table SQL, le LDAP, etc) et on laisse la classe `Compose` agréger les résultats. - On avait déjà une classe `ModelSearch` - J'ajoute une classe `LDAPSearch` avec une interface similaire - `Compose` fait le taf d'appeler tout le monde et de virer les doublons. À propos des doublons : je ne suis pas hyper convaincu de ce que j'ai fait mais j'ai pas tellement de meilleure idée, en tout cas pas qui soit aussi simple. Des idées ou on garde ça ? À voir ensemble (au moins) : - [x] Est-ce que le design général est ok ? - [x] Est-ce que les docstrings sont claires ?
mpepin commented 2019-12-21 12:32:22 +01:00 (Migrated from git.eleves.ens.fr)
Copy link

added 1 commit

  • 885a1889 - COF autocompletion: more direct definition style

Compare with previous version

added 1 commit <ul><li>885a1889 - COF autocompletion: more direct definition style</li></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/390/diffs?diff_id=1411&start_sha=5f596aafc6483902d31bd95f50853eb4fd22df54)
mpepin commented 2019-12-21 12:58:54 +01:00 (Migrated from git.eleves.ens.fr)
Copy link

mentioned in merge request !396

mentioned in merge request !396
lstephan commented 2019-12-23 09:53:13 +01:00 (Migrated from git.eleves.ens.fr)
Copy link

Après réflexion, je serais plutôt pour fermer cette MR...

J'ai l'impression que c'est à la fois très général (tous les trucs de _get_clipper_field par exemple), mais que finalement le usecase est assez restreint. Typiquement, sur les 3/4 utilisations de l'autocomplétion (comptes K-Fêt, création de compte K-Fêt, inscription d'utilisateurs, [future] recherche de participants BdA (voir #235)) dans le gestioCOF actuel, ça n'en couvre vraiment qu'un seul.

Ce que j'aurais plus vu, quitte à factoriser l'autocomplétion, ça serait une vue (là, une TemplateView me va très bien) qui gère l'affichage, et qui délègue l'autocomplétion à des modules plus spécialisés (clippers, comptes COF, comptes K-Fêt, etc). Mais il y a aussi probablement d'autres solutions =)

En tout cas, je pense que ça mérite un peu de discussion sur le sujet ; si @champeno ou @areitz ont des idées...

Après réflexion, je serais plutôt pour fermer cette MR... J'ai l'impression que c'est à la fois très général (tous les trucs de `_get_clipper_field` par exemple), mais que finalement le usecase est assez restreint. Typiquement, sur les 3/4 utilisations de l'autocomplétion (comptes K-Fêt, création de compte K-Fêt, inscription d'utilisateurs, [future] recherche de participants BdA (voir #235)) dans le gestioCOF actuel, ça n'en couvre vraiment qu'un seul. Ce que j'aurais plus vu, quitte à factoriser l'autocomplétion, ça serait une vue (là, une `TemplateView` me va très bien) qui gère l'affichage, et qui délègue l'autocomplétion à des modules plus spécialisés (clippers, comptes COF, comptes K-Fêt, etc). Mais il y a aussi probablement d'autres solutions =) En tout cas, je pense que ça mérite un peu de discussion sur le sujet ; si @champeno ou @areitz ont des idées...
mpepin commented 2019-12-23 16:19:25 +01:00 (Migrated from git.eleves.ens.fr)
Copy link

Le but était effectivement de tout factoriser, et je pense que ça couvre plus ou moins les inscriptions COF/BDS/K-Fêt. Mais comme tu dis, c'est encore un peu trop rigide. Je suis d'accord pour qu'on prenne le temps de faire un truc mieux.

Ta suggestion me paraît bien, un truc me fait peur : il ne faut pas garder la logique de l'autocomplétion des différentes apps séparées comme c'est le cas actuellement. Mais c'est peut-être pas ce que tu sous-entendais. Au minimum il faut que la recherche dans le LDAP soit partagée par tout le monde, mais idéalement il faudrait un moyen de "composer" des modes de recherche, un peu dans la même idée que le _get_clipper_field mais de façon plus générique. Peut-être avec une liste de champs dans lesquels chercher, à la manière de https://git.eleves.ens.fr/klub-dev-ens/gestioCOF/blob/master/gestioncof/views.py#L945. J'essaie de creuser cette piste.

Le but était effectivement de tout factoriser, et je pense que ça couvre plus ou moins les inscriptions COF/BDS/K-Fêt. Mais comme tu dis, c'est encore un peu trop rigide. Je suis d'accord pour qu'on prenne le temps de faire un truc mieux. Ta suggestion me paraît bien, un truc me fait peur : il ne faut pas garder la logique de l'autocomplétion des différentes apps séparées comme c'est le cas actuellement. Mais c'est peut-être pas ce que tu sous-entendais. Au minimum il faut que la recherche dans le LDAP soit partagée par tout le monde, mais idéalement il faudrait un moyen de "composer" des modes de recherche, un peu dans la même idée que le `_get_clipper_field` mais de façon plus générique. Peut-être avec une liste de champs dans lesquels chercher, à la manière de https://git.eleves.ens.fr/klub-dev-ens/gestioCOF/blob/master/gestioncof/views.py#L945. J'essaie de creuser cette piste.
lstephan commented 2019-12-23 19:08:09 +01:00 (Migrated from git.eleves.ens.fr)
Copy link

Ce que je sous entendais serait plutôt une logique du genre :

  • une vue d'autocomplétion (qui serait en gros ta classe Autocomplete), qui s'occupe juste de l'affichage (et est indépendante de toutes les apps)
  • plusieurs "providers" (nom à étudier) dont le but est de fournir de l'autocomplétion dans leur domaine précis. Parmi eux, il peut y avoir des trucs app-indépendant (genre le LDAP), ou dépendant (genre la recherche de compte K-Fêt)

Le but de la vue d'affichage serait juste d'agréger les complétions de ses différents providers en un truc unique, et du coup à chaque fois qu'on veut utiliser de l'autocomplétion il suffit de subclass Autocomplete et de lui spécifier les providers nécessaires.

Ce que je sous entendais serait plutôt une logique du genre : * une vue d'autocomplétion (qui serait en gros ta classe `Autocomplete`), qui s'occupe juste de l'affichage (et est indépendante de toutes les apps) * plusieurs "providers" (nom à étudier) dont le but est de fournir de l'autocomplétion dans leur domaine précis. Parmi eux, il peut y avoir des trucs app-indépendant (genre le LDAP), ou dépendant (genre la recherche de compte K-Fêt) Le but de la vue d'affichage serait juste d'agréger les complétions de ses différents providers en un truc unique, et du coup à chaque fois qu'on veut utiliser de l'autocomplétion il suffit de subclass `Autocomplete` et de lui spécifier les providers nécessaires.
mpepin commented 2020-01-02 17:39:37 +01:00 (Migrated from git.eleves.ens.fr)
Copy link

mentioned in merge request !401

mentioned in merge request !401
mpepin commented 2020-05-07 14:50:20 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

added 93 commits

  • 885a1889...c8b8c905 - 87 commits from branch master
  • 914888d1 - Merge the utils and shared apps
  • d2c6c9da - Type hints in shared.views.autocomplete
  • e45ee3fb - More documentation for ModelSearch
  • a259b04d - Explicative comment about the Type[M] annotation
  • b8cd5f1d - Drop type hints in shared.views.autocomplete
  • b1d8bb04 - Generic auto-completion mechanism

Compare with previous version

added 93 commits <ul><li>885a1889...c8b8c905 - 87 commits from branch <code>master</code></li><li>914888d1 - Merge the utils and shared apps</li><li>d2c6c9da - Type hints in shared.views.autocomplete</li><li>e45ee3fb - More documentation for ModelSearch</li><li>a259b04d - Explicative comment about the Type[M] annotation</li><li>b8cd5f1d - Drop type hints in shared.views.autocomplete</li><li>b1d8bb04 - Generic auto-completion mechanism</li></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/390/diffs?diff_id=1586&start_sha=885a188939e2774e5037c781c2699c8dfd56e695)
mpepin commented 2020-05-07 15:19:32 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

changed the description

changed the description
mpepin commented 2020-05-07 15:19:32 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

assigned to @lstephan

assigned to @lstephan
mpepin commented 2020-05-07 15:20:24 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

J'ai implémenté en gros ce que tu disais. Ça te parait comment ?

J'ai implémenté en gros ce que tu disais. Ça te parait comment ?
lstephan commented 2020-05-07 15:23:42 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

La syntaxe whitespace-separated est assez perturbante imo, ['clipper', 'fullname'] serait plus lisible.

La syntaxe whitespace-separated est assez perturbante imo, `['clipper', 'fullname']` serait plus lisible.
mpepin commented 2020-05-07 15:25:43 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

Oh j'avais pas vu qu'on pouvais faire ça, c'est beaucoup mieux oui

Oh j'avais pas vu qu'on pouvais faire ça, c'est beaucoup mieux oui
lstephan commented 2020-05-07 15:25:56 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

À mettre dans la docstring de la fonction du dessus ?

À mettre dans la docstring de la fonction du dessus ?
lstephan commented 2020-05-07 15:26:18 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

typo =)

typo =)
lstephan commented 2020-05-07 15:26:59 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

C'est pep-standard de mettre les double ` dans les docstrings ?

C'est pep-standard de mettre les double \` dans les docstrings ?
mpepin commented 2020-05-07 15:31:52 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

aucune idée, j'ai l'impression qu'on voit un peu de tout dans la nature mais j'ai pas eu la motiv d'aller chercher dans les PEPs

aucune idée, j'ai l'impression qu'on voit un peu de tout dans la nature mais j'ai pas eu la motiv d'aller chercher dans les PEPs
mpepin commented 2020-05-07 15:31:59 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

thx!

thx!
lstephan commented 2020-05-07 15:34:02 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

Ça me paraît vraiment bien ! Dernier caveat (qui est possiblement trop overengineer ?) : est ce que tu penses que ça serait une bonne idée de déléguer une partie du rendering aux sous classes (ou à la classe d'agrégation) ? J'ai l'impression que la présentations des résultats varie très peu d'une autocomplétion à l'autre, mais peut-être que les légères différences rendent ça relou...

Ça me paraît vraiment bien ! Dernier caveat (qui est possiblement trop overengineer ?) : est ce que tu penses que ça serait une bonne idée de déléguer une partie du rendering aux sous classes (ou à la classe d'agrégation) ? J'ai l'impression que la présentations des résultats varie très peu d'une autocomplétion à l'autre, mais peut-être que les légères différences rendent ça relou...
mpepin commented 2020-05-07 15:34:30 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

Une seule ` tu préfères ? Moi aussi, je ne sais plus où j'ai pris cette habitude des `` mais c'est moche

Une seule ` tu préfères ? Moi aussi, je ne sais plus où j'ai pris cette habitude des `` mais c'est moche
lstephan commented 2020-05-07 15:35:15 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

Je trouve ça un peu lourd avec ``, effectivement...

Je trouve ça un peu lourd avec \`\`, effectivement...
mpepin commented 2020-05-07 15:44:19 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

C'est une bonne question.

J'ai pas de réponse définitive mais je pense que ça prendra la forme d'une class-based view AutocompletionView, qui utilise Compose en interne, mais séparée dans une autre classe.
Donc a priori c'est facile de travailler sans en attendant et facile à mettre dans une autre MR, je propose d'en rester là pour cette MR, si ça te va

C'est une bonne question. J'ai pas de réponse définitive mais je pense que ça prendra la forme d'une class-based view `AutocompletionView`, qui utilise `Compose` en interne, mais séparée dans une autre classe. Donc a priori c'est facile de travailler sans en attendant et facile à mettre dans une autre MR, je propose d'en rester là pour cette MR, si ça te va
mpepin commented 2020-05-07 15:45:23 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

changed this line in version 5 of the diff

changed this line in [version 5 of the diff](/klub-dev-ens/gestioCOF/merge_requests/390/diffs?diff_id=1588&start_sha=b1d8bb04c4d9e772c4cc205de22147b893e01991#9887b4f3bd1e903210c6de540a018cc2a0231777_93_93)
mpepin commented 2020-05-07 15:45:23 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

changed this line in version 5 of the diff

changed this line in [version 5 of the diff](/klub-dev-ens/gestioCOF/merge_requests/390/diffs?diff_id=1588&start_sha=b1d8bb04c4d9e772c4cc205de22147b893e01991#9887b4f3bd1e903210c6de540a018cc2a0231777_120_124)
mpepin commented 2020-05-07 15:45:24 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

changed this line in version 5 of the diff

changed this line in [version 5 of the diff](/klub-dev-ens/gestioCOF/merge_requests/390/diffs?diff_id=1588&start_sha=b1d8bb04c4d9e772c4cc205de22147b893e01991#9887b4f3bd1e903210c6de540a018cc2a0231777_146_147)
mpepin commented 2020-05-07 15:45:24 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

added 1 commit

Compare with previous version

added 1 commit <ul><li>3b0d4ba5 - lstephan&#39;s suggestions</li></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/390/diffs?diff_id=1588&start_sha=b1d8bb04c4d9e772c4cc205de22147b893e01991)
mpepin commented 2020-05-07 15:45:46 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

done

done
lstephan commented 2020-05-07 16:01:51 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

Pourquoi s'emmerder à map si on ne fait que les compter ?

Pourquoi s'emmerder à `map` si on ne fait que les compter ?
lstephan commented 2020-05-07 16:28:10 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

marked the checklist item Est-ce que le design général est ok ? as completed

marked the checklist item **Est-ce que le design général est ok ?** as completed
lstephan commented 2020-05-07 16:28:11 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

marked the checklist item Est-ce que les docstrings sont claires ? as completed

marked the checklist item **Est-ce que les docstrings sont claires ?** as completed
lstephan commented 2020-05-07 16:28:41 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

resolved all discussions

resolved all discussions
lstephan commented 2020-05-07 16:28:47 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

merged

merged
lstephan commented 2020-05-07 16:28:48 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

mentioned in commit 64ceb813c6

mentioned in commit 64ceb813c687dcbcb411753228b05a3a929fd49e
mpepin commented 2020-05-07 18:04:57 +02:00 (Migrated from git.eleves.ens.fr)
Copy link

Le nom est trompeur : assertCountEqual vérifie que les deux itérables contiennent les mêmes éléments avec le même nombre de répétitions. Et c'est implémenté avec un collections.Counter d'où le nom.

Ref: https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertCountEqual

Le nom est trompeur : `assertCountEqual` vérifie que les deux itérables contiennent les mêmes éléments avec le même nombre de répétitions. Et c'est implémenté avec un `collections.Counter` d'où le nom. Ref: https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertCountEqual
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
devtype -- backend

Au programme : du Python, du Django...

  
devtype -- docs

   
devtype -- frontend

   
devtype -- user interface

Au programme : templating, JS, CSS...

  
difficulty -- easy

   
difficulty -- hard

Difficile et/ou risqué

  
difficulty -- normal

   
Doing

   
domain -- bda

Concerne l'application BdA

  
domain -- bds

   
domain -- cof

Concerne l'application COF

  
domain -- core

Dépendances externes, CI, linting...

  
domain -- kfet

Concerne l'application K-Fêt

  
Good first issue

   
priority -- high

Urgent quoi

  
priority -- low

   
priority -- medium

   
priority -- staff-wanted

L'équipe dirigeante du domaine en question souhaite cette fonctionnalité.

  
status -- development

En cours de développement

  
status -- discussion

Avant de coder, c'est bien de discuter

  
status -- need review

En attente de code review

  
status -- production

L'ajout/la modification est présente en production

  
status -- ready to merge

Plus qu'à merger !

  
status -- todo

Le développement devrait bientôt commencer

  
To Do

   
type -- bug

Signalement ou correction de bug

  
type -- hygiene

Tout ce qui amène à du code plus propre et un repo mieux organisé.

  
type -- improvement

Ajout/Demande d'évolution mineure d'une fonctionnalité existante

  
type -- new feature

Ajout/Demande d'une nouvelle fonctionnalité

No labels
devtype -- backend
devtype -- docs
devtype -- frontend
devtype -- user interface
difficulty -- easy
difficulty -- hard
difficulty -- normal
Doing
domain -- bda
domain -- bds
domain -- cof
domain -- core
domain -- kfet
Good first issue
priority -- high
priority -- low
priority -- medium
priority -- staff-wanted
status -- development
status -- discussion
status -- need review
status -- production
status -- ready to merge
status -- todo
To Do
type -- bug
type -- hygiene
type -- improvement
type -- new feature
Milestone
Clear milestone
No items
No milestone
Intégration du BDS
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: DGNum/gestioCOF#694
No description provided.
Delete branch "kerl/autocomplete"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?