Préparation de la vue d'autocompletion pour l'intégration du BDS

mpepin commented

2020-01-02 17:39:28 +01:00

(Migrated from git.eleves.ens.fr)

Petits changements pour préparer !390

Fusion des modules shared et utils : je ne sais plus comment on s'est retrouvé avec deux modules séparés mais ça ne me paraît mieux de (re)fusionner.
Séparation entre :
- la recherche (classe ModelSearch)
- et la vue django-autocomplete-light (classe Select2QuerySetView).
L'idée et qu'on puisse garder la vue d'autocomplétion dal et réutiliser la partie "recherche" dans !390 avec un "combinateur" qui agrège les résultats de plusieurs instances de ModelSearch, les résultats d'une recherche ldap, etc. cf https://git.eleves.ens.fr/klub-dev-ens/gestioCOF/merge_requests/390#note_6718.
Bonus : annotations de type dans shared.views.autocomplete. Je ne sais pas si on veut faire ça partout, ou à quelques endroits, ou nulle part. Je mets ça là pour montrer à quoi ça peut ressembler, je pense que dans ce genre de cas ça fait de la doc pas chère. C'est un tout petit commit, aucun souci pour le virer si on décide qu'on n'aime pas.

Petits changements pour préparer !390 - Fusion des modules `shared` et `utils` : je ne sais plus comment on s'est retrouvé avec deux modules séparés mais ça ne me paraît mieux de (re)fusionner. - Séparation entre : - la recherche (classe `ModelSearch`) - et la vue django-autocomplete-light (classe `Select2QuerySetView`). L'idée et qu'on puisse garder la vue d'autocomplétion dal et réutiliser la partie "recherche" dans !390 avec un "combinateur" qui agrège les résultats de plusieurs instances de `ModelSearch`, les résultats d'une recherche ldap, etc. cf https://git.eleves.ens.fr/klub-dev-ens/gestioCOF/merge_requests/390#note_6718. - *Bonus* : [annotations de type](https://docs.python.org/3/library/typing.html) dans `shared.views.autocomplete`. Je ne sais pas si on veut faire ça partout, ou à quelques endroits, ou nulle part. Je mets ça là pour montrer à quoi ça peut ressembler, je pense que dans ce genre de cas ça fait de la doc pas chère. C'est un tout petit commit, aucun souci pour le virer si on décide qu'on n'aime pas.

mpepin commented

2020-01-02 17:39:28 +01:00

(Migrated from git.eleves.ens.fr)

changed milestone to %1

lstephan commented

2020-01-02 18:14:21 +01:00

(Migrated from git.eleves.ens.fr)

Pourquoi il faut créer une TypeVarpour Model et pas pour Q ?

Pourquoi il faut créer une `TypeVar`pour `Model` et pas pour `Q` ?

mpepin commented

2020-01-02 20:57:06 +01:00

(Migrated from git.eleves.ens.fr)

Oui, M est une variable de type (avec la contrainte d'être un sous type de Model), car ça peut être n'importe quel modèle. En revanche le résultat de get_queryset_filter c'est toujours un Q.

Si j'ai bien compris la sémantique de typing.Type, la ligne 16 sert à dire que model = M et donc que les résultats de la recherche sont des instances de model.

Oui, `M` est une variable de type (avec la contrainte d'être un sous type de `Model`), car ça peut être n'importe quel modèle. En revanche le résultat de `get_queryset_filter` c'est toujours un `Q`. Si j'ai bien compris la sémantique de `typing.Type`, la ligne 16 sert à dire que `model = M` et donc que les résultats de la recherche sont des instances de `model`.

mpepin commented

2020-01-02 21:06:57 +01:00

(Migrated from git.eleves.ens.fr)

À noter que la CI ne passe pas pour une bonne raison : la VM (ainsi que le conteneur de la CI) sont sous python 3.5 qui ne supporte les annotations de type que dans les déclarations de fonctions, pas pour les classes.

La machine COF est sous debian oldstable, passer sous stable réglerait le problème (stable est sous python 3.7).

Soit on fait l'upgrade bientôt, soit ça prends du temps et on peut laisser tomber les annotations pour cette fois.

À noter que la CI ne passe pas pour une bonne raison : la VM (ainsi que le conteneur de la CI) sont sous python 3.5 qui ne supporte les annotations de type que dans les déclarations de fonctions, pas pour les classes. La machine COF est sous debian oldstable, passer sous stable réglerait le problème (stable est sous python 3.7). Soit on fait l'upgrade bientôt, soit ça prends du temps et on peut laisser tomber les annotations pour cette fois.

lstephan commented

2020-01-02 22:14:12 +01:00

(Migrated from git.eleves.ens.fr)

Ca pourrait valoir le coup d'être commenté, du coup =)

lstephan commented

2020-01-03 01:12:06 +01:00

(Migrated from git.eleves.ens.fr)

Perso j'aime bien les type hints (module quelques comments, du coup), mais je sais pas à quel point ça prend du temps d'upgrade =)

lstephan commented

2020-01-03 01:28:47 +01:00

(Migrated from git.eleves.ens.fr)

Après avoir un peu bouquiné la doc, j'ai l'impression que c'est redondant : la définition de M via TypeVar dit déjà que M contient toutes les sous-classes de Model, non ? Il aurait fallu mettre Type[Model] sinon, mais vu qeu tu as besoin de M pour le type générique...

Après avoir un peu bouquiné la doc, j'ai l'impression que c'est redondant : la définition de `M` via `TypeVar` dit déjà que `M` contient toutes les sous-classes de `Model`, non ? Il aurait fallu mettre `Type[Model]` sinon, mais vu qeu tu as besoin de `M` pour le type générique...

mpepin commented

2020-01-03 16:08:47 +01:00

(Migrated from git.eleves.ens.fr)

Non, M ne contient pas toutes les sous classes de Model. M est une variable qui peut être substituée par n'importe quelle type (ou classe en l'occurrence) qui est un sous-type (qui hérite) de Model. Ainsi, la substitution de M par User donne ModelSearch[User] qui est le type des instances qui renvoient uniquement des Users, de même ModelSearch[Account] est le type des instances qui renvoient des Accounts, etc.

Le fait de mettre l'annotation model: Type[M] indique que le type des objets renvoyés par search correspond bien au type indiqué dans l'attribut model.

J'aurais aussi pu mettre search(self, keywords: Iterable[str]) -> Iterable[Model] et model: Type[Model] mais dans ce cas on est moins précis : on indique juste que la classe renvoie des instances de modèles, on ne dit pas de quels modèles.

Par exemple :

class UserSearch(ModelSearch):
    model = User
    search_fields = ["username", "first_name", "last_name"]

user_search = UserSearch()
user: User = user_search.search(["toto"])  # type-check
wrong: Account = user_search.search(["toto"])  # type-error

Non, `M` ne *contient* pas toutes les sous classes de `Model`. `M` est une variable qui peut être *substituée* par n'importe quelle type (ou classe en l'occurrence) qui est un sous-type (qui hérite) de `Model`. Ainsi, la substitution de `M` par `User` donne `ModelSearch[User]` qui est le type des instances qui renvoient uniquement des `User`s, de même `ModelSearch[Account]` est le type des instances qui renvoient des `Account`s, etc. Le fait de mettre l'annotation `model: Type[M]` indique que le type des objets renvoyés par `search` correspond bien au type indiqué dans l'attribut `model`. J'aurais aussi pu mettre `search(self, keywords: Iterable[str]) -> Iterable[Model]` et `model: Type[Model]` mais dans ce cas on est moins précis : on indique juste que la classe renvoie des instances de modèles, on ne dit pas de quels modèles. Par exemple : ```python class UserSearch(ModelSearch): model = User search_fields = ["username", "first_name", "last_name"] user_search = UserSearch() user: User = user_search.search(["toto"]) # type-check wrong: Account = user_search.search(["toto"]) # type-error ```

mpepin commented

2020-01-03 17:09:20 +01:00

(Migrated from git.eleves.ens.fr)

C'est probablement rendu peu clair par le fait qu'en python il faut créer ses variables de type soi-même alors que dans la plupart des langages avec du polymorphisme, ça fait partie de la syntaxe. Dis toi que la déclaration de la variable de type M ligne 6 ne devrait pas exister de même qu'on ne déclare pas à l'avance les arguments des fonctions, on écrit juste def add(x, y): ... et x et y existent dans le corps de add.

C'est probablement rendu peu clair par le fait qu'en python il faut créer ses variables de type soi-même alors que dans la plupart des langages avec du polymorphisme, ça fait partie de la syntaxe. Dis toi que la déclaration de la variable de type `M` ligne 6 ne devrait pas exister de même qu'on ne déclare pas à l'avance les arguments des fonctions, on écrit juste `def add(x, y): ...` et `x` et `y` existent dans le corps de `add`.

mpepin commented

2020-01-03 17:27:02 +01:00

(Migrated from git.eleves.ens.fr)

added 1 commit

a20f7de3 - More documentation for ModelSearch

Compare with previous version

added 1 commit <ul><li>a20f7de3 - More documentation for ModelSearch</li></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/401/diffs?diff_id=1445&start_sha=23d87bab17f19c209585afae2053c2e96eb9f339)

mpepin commented

2020-01-03 17:30:52 +01:00

(Migrated from git.eleves.ens.fr)

added 1 commit

c5b69f9f - Explicative comment about the Type[M] annotation

Compare with previous version

added 1 commit <ul><li>c5b69f9f - Explicative comment about the Type[M] annotation</li></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/401/diffs?diff_id=1446&start_sha=a20f7de3e6e4e55d5a15cf5549679a09b3aa1730)

mpepin commented

2020-01-04 17:29:16 +01:00

(Migrated from git.eleves.ens.fr)

marked as a Work In Progress

marked as a **Work In Progress**

mpepin commented

2020-01-04 17:29:16 +01:00

(Migrated from git.eleves.ens.fr)

changed the description

mpepin commented

2020-01-27 21:37:48 +01:00

(Migrated from git.eleves.ens.fr)

Bon, deux choses :

Vu la longueur de la conversation plus haut, les annotations (dans ce cas au moins) sont source d'incompréhension alors qu'elles devraient aider à comprendre le code. On devrait sûrement les virer.
Si on vire les annotations, on peut merge sans attendre que la machine COF soit mise à jour.

Tu valides @lstephan ?

Bon, deux choses : 1. Vu la longueur de la conversation plus haut, les annotations (dans ce cas au moins) sont source d'incompréhension alors qu'elles devraient aider à comprendre le code. On devrait sûrement les virer. 2. Si on vire les annotations, on peut merge sans attendre que la machine COF soit mise à jour. Tu valides @lstephan ?

mpepin commented

2020-02-12 19:01:51 +01:00

(Migrated from git.eleves.ens.fr)

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/klub-dev-ens/gestioCOF/merge_requests/401/diffs?diff_id=1499&start_sha=c5b69f9f2a5fe73811968379344db54e95e2e967#9887b4f3bd1e903210c6de540a018cc2a0231777_36_27)

mpepin commented

2020-02-12 19:01:53 +01:00

(Migrated from git.eleves.ens.fr)

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/klub-dev-ens/gestioCOF/merge_requests/401/diffs?diff_id=1499&start_sha=c5b69f9f2a5fe73811968379344db54e95e2e967#9887b4f3bd1e903210c6de540a018cc2a0231777_33_24)

mpepin commented

2020-02-12 19:01:54 +01:00

(Migrated from git.eleves.ens.fr)

added 1 commit

5fea7a1b - Drop type hints in shared.views.autocomplete

Compare with previous version

added 1 commit <ul><li>5fea7a1b - Drop type hints in shared.views.autocomplete</li></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/401/diffs?diff_id=1499&start_sha=c5b69f9f2a5fe73811968379344db54e95e2e967)

mpepin commented

2020-02-12 19:03:15 +01:00

(Migrated from git.eleves.ens.fr)

unmarked as a Work In Progress

unmarked as a **Work In Progress**

mpepin commented

2020-02-12 19:04:31 +01:00

(Migrated from git.eleves.ens.fr)

added 39 commits

5fea7a1b...494cd5dd - 34 commits from branch master
6467320b - Merge the utils and shared apps
a3245301 - Type hints in shared.views.autocomplete
7f4706e8 - More documentation for ModelSearch
7a6f0d46 - Explicative comment about the Type[M] annotation
387dca22 - Drop type hints in shared.views.autocomplete

Compare with previous version

added 39 commits <ul><li>5fea7a1b...494cd5dd - 34 commits from branch <code>master</code></li><li>6467320b - Merge the utils and shared apps</li><li>a3245301 - Type hints in shared.views.autocomplete</li><li>7f4706e8 - More documentation for ModelSearch</li><li>7a6f0d46 - Explicative comment about the Type[M] annotation</li><li>387dca22 - Drop type hints in shared.views.autocomplete</li></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/401/diffs?diff_id=1500&start_sha=5fea7a1b2fdf297db556d7e14401d3968f4db945)

mpepin commented

2020-05-05 22:34:19 +02:00

(Migrated from git.eleves.ens.fr)

added 33 commits

387dca22...c8b8c905 - 28 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

Compare with previous version

added 33 commits <ul><li>387dca22...c8b8c905 - 28 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></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/401/diffs?diff_id=1582&start_sha=387dca227abd15891c6ee98317bf8fbff9da0b8d)

mpepin commented

2020-05-05 22:35:51 +02:00

(Migrated from git.eleves.ens.fr)

plus de types

mpepin commented

2020-05-05 22:36:08 +02:00

(Migrated from git.eleves.ens.fr)

plus de types

mpepin commented

2020-05-05 22:36:19 +02:00

(Migrated from git.eleves.ens.fr)

resolved all discussions

mpepin commented

2020-05-05 22:37:16 +02:00

(Migrated from git.eleves.ens.fr)

@lstephan J'ai viré typing puisque ça semblait plus source de confusion qu'autre chose.

Est-ce que ça te va en l'état ?

@lstephan J'ai viré typing puisque ça semblait plus source de confusion qu'autre chose. Est-ce que ça te va en l'état ?

lstephan commented

2020-05-07 14:54:43 +02:00

(Migrated from git.eleves.ens.fr)

Il me semblait qu'on avait parlé de faire une classe plus générale donc ModelSearch hériterait, mais ça peut attendre une autre MR, donc je merge ça =)

Il me semblait qu'on avait parlé de faire une classe plus générale donc `ModelSearch` hériterait, mais ça peut attendre une autre MR, donc je merge ça =)

lstephan commented

2020-05-07 14:54:53 +02:00

(Migrated from git.eleves.ens.fr)

merged

lstephan commented

2020-05-07 14:54:53 +02:00

(Migrated from git.eleves.ens.fr)

mentioned in commit 5298a19667

mentioned in commit 5298a1966721957c15533fd3d107bf9567106ff3

mpepin commented

2020-05-07 15:01:23 +02:00

(Migrated from git.eleves.ens.fr)

yep, ça vient juste après ;)

Préparation de la vue d'autocompletion pour l'intégration du BDS #705