Produits Entr'ouvert » Combo

Frédéric Péters a écrit :

Dans la branche, mais qui gagne son propre ticket, j'ai aussi fait #21167.

Ça m'irait bien maintenant que les trois commits marqués #21052 dans la branche https://git.entrouvert.org/combo.git/log/?h=wip/performances soient relus. (je les attache à ce ticket)

• 0001: il reste un truc un peu lent à cause de Page.get_parents_and_self() qui continue à faire du pointer chasing mais c'est à voir un autre jour en changeant la façon de gérer l'arbre. Pour le reste ça m'a l'air bon, je paraphrase le code pour voir si j'ai bien compris:
  • on récupère la liste des pages parents avant de résoudre les ParentContentCell,
  • ensuite on récupère d'un coup les cellules de toutes les pages parents + la page index,
  • on séparer les cellules de la page parent de la page en cours dans une variable cells
  • on remonte la chaîne des pages parentes pour chacune:
    • on parcourt cells si on trouve un ParentContentCell on remplace par les cellules de la page parent en cours d'analyse.

for cell in cells:
   if isinstance(cell, ParentContentCell):
      # replace by parent page cells
      cells[i:i + 1] = cells_by_page[page_id]
      break
else: # no more ParentContentCell, stop following the parent page chain
   break

• 0002: Le catch de DoesNotExist sur Page(slug='index') et le reste du code dans ParentContentCell.get_cells() me semble appartenir au patch précédent. Pour le reste je séparerai le patch en 2, les améliorations à l'API de CellBase puis l'application au problème de trouver les cellules qui ont une méthode get_badge(). Aussi peut-être que ce serait moins impactant d'ajouter un class_filter à CellBase.get_cells() ça évite de recopier le parcours de classe partout (for klass in CellBase.get_cell_classes(...): cells.extend(...)) qui jusqu'à présent était bien encapsulé dans CellBase.get_cells(). Je ne vois pas le rapport des modifications dans les tests de lingo, faut en faire un patch à part et si ça a un rapport dire lequel.
• 0003: et donc ici avec mes suggestions le code pourrait devenir simplement

  other_cells = CellBase.get_cells(page_id=cell.page_id, class_filter=lambda k: bool(k.modify_global_context), cell_filter=lambda c: c.is_visible(user=request.user))

  , ça supprime 4 lignes. À voir si on gagne vraiment à ne pas résoudre cell.page en utilisant cell.page_id (j'ai l'impression qu'on va forcément faire une requête pour la page à un moment, un select_related('page') aux bons endroits suffirait à ce qu'on ne se pose plus la question).

• 0001 , https://git.entrouvert.org/combo.git/commit/?h=wip/performances&id=a485cf8c72e2dd4ff8355bc2528910169a52dee1

Je verrai comme optimisation en plus d'arrêter le parcours des pages parents [...]

Yep.

Je renommerai bien la deuxième en get_parent_cells() pour éviter les incompréhensions.

J'ai mis get_parents_cells() (parentS) mais oui, tout à fait plus clair ainsi.

ça évite de passer la valeur un peu partout [...] on évite ainsi de modifier toutes les signatures pour y passer hierarchy.

Ça reste quand même limité à deux/trois appels, je préfère rester explicite ici qu'ajouter la mise en cache.

Et tant qu'à optimiser le nombre de requêtes il faudrait un test sur ce point.

Yep, ajouté. Une requête SQL en plus par niveau dans la hiérarchie.

• 0002, splitté en https://git.entrouvert.org/combo.git/commit/?h=wip/performances&id=ecf47358aaaee80c0047c0431068f37a0d98b4e3 & https://git.entrouvert.org/combo.git/commit/?h=wip/performances&id=8c4fa059fc2fa36701cc20c737f5c07fb28816ad

Oui, j'ai déplacé correctement ce qui relevait du 0001 dans le précédent. Et les tests lingo je n'ai plus le souvenir précis mais déplacés dans leur propre commit. Et limité un premier commit à "refactor get_all_cell_types to support a filtering function" sans toucher aux badges.

Par contre je n'essaie pas d'unifier le parcours des classes, il reste quelques mini différences dont je n'évalue pas l'impact. (dans un cas on prête attention au flag "visible" qu'on utilise pour ne pas présenter dans l'UI du /manage/ des cellules dépréciées, mais dans le cas du rendu de la page, on passe quand même dessus parce qu'on veut effectivement conserver le rendu de la cellule si elle était déjà là) (et c'est parce que je ne suis pas du tout sûr que ça soit réellement ça la raison, je préfère ne pas toucher).

• 0003, https://git.entrouvert.org/combo.git/commit/?h=wip/performances&id=7d142556de21c958016d90cb0ab08cc442b3e989

Au vu de mon commentaire précédent, je le laisse en l'état.

Ack

J'écrivais :

Il faut vraiment inclure uniquement la réponse dans les commentaires redmine, autrement ça fait un truc tout à fait illisible. (tu peux éditer les commentaires pour corriger ça)

Et je l'ai fait moi-même.