Produits Entr'ouvert » Passerelle

• Il faut reporter la modif sur cmis_endpoint (verbose_name=_('CMIS Atom endpoint')) dans cmis/migrations/0001_initial.py (pour pas que django croit qu'il manque des migrations).
• idem avec le help_text, au passage y'a une espace en trop dans help_text=_('URL of the CMIS Atom endpoint')
• sur la forme, on s'autorise des lignes de 100 colonnes, donc tu pourrais avoir moins de retour à la ligne sans doute avec cette limite (vim: au FileType python setl autoindent tabstop=4 expandtab shiftwidth=4 softtabstop=4 textwidth=99 fileformat=unix nu)
• pas de assert en dehors des tests, même si les copains en font, je fais toujours les gros yeux. Parce qu'en prod (-O2) les assert sont juste ignorés par Python.

Thomas Noël a écrit :

• pas de assert en dehors des tests, même si les copains en font, je fais toujours les gros yeux. Parce qu'en prod (-O2) les assert sont juste ignorés par Python.

Je plussoie le code est tout aussi clair sous cette forme:

def validate_input(self, input):
    try;
        data = json.loads(input)
    except ValueError:
        return None, "input is not JSON" 
    if not condition1(data):
        return None, "condition1 not present" 
    etc..
    return data, None

• # httplib2 will probably be used instead of urllib si tu connais déjà l'exception qu'on recevra ça me va de l'y mettre tout de suite, en laissant un commentaire du même genre.

• je trouve assez bizarre que file_name, file_path et file_byte_content soient des paramètres du constructeur de CMISGateway, je verrai bien ça déporté au niveau de la méthode correspondante et plutôt que de répéter son instanciation je verrai bien une propriété cmis_gateway sur le modèle qui s'occupe de construire l'objet, idéalement je verrai tout ce qui concerne cmislib dans CMISGateway le reste dans les méthodes de endpoint du modèle, histoire d'avoir deux couches distrinctes (quitte à créer une exception CMISGatewayError et à ne gérer que ça au niveau du modèle, ce serait le boulot de CMISGateway de tout cacher)

• Je ne suis pas certain mais je crois qu'au lieu de:
  

      folder = Mock(**{'createDocument.return_value': 'doc'})
  

  
  on doit pouvoir écrire
  

      folder = Mock()
      folder.createdDocument.return_value = 'doc'
  

qui prend certes une ligne de plus mais qui me semble plus idiomatique.

• J'apprécie beaucoup la profondeur des tests :)

Benjamin Dauvergne a écrit :

• J'apprécie beaucoup la profondeur des tests :)

Tout à mes critiques négatives, j'ai oublié de dire que moi aussi j'ai beaucoup plein aimé les tests.

D'autres remarques quelqu'un ? Ack ?

Back , je te dis quoi.

Emmanuel Cazenave a écrit :

Nouveau patch.J'ai tenu compte de toutes vos remarques modulo les miennes ci-dessous.

Benjamin Dauvergne a écrit :

• # httplib2 will probably be used instead of urllib si tu connais déjà l'exception qu'on recevra ça me va de l'y mettre tout de suite, en laissant un commentaire du même genre.

ça me fait bizarre d'introduire une nouvelle dépendance à cause la future version d'une lib dont on ne dépend pas encore...mais je peux le faire si tu y tiens.

Je ne sais pas trop quand tu penses qu'on passera à httplib2 (Python 3) ?

...(quitte à créer une exception CMISGatewayError et à ne gérer que ça au niveau du modèle, ce serait le boulot de CMISGateway de tout cacher)

Tout à fait d'accord pour l'encapsulation mais il me semble overkill de rajouter à CMISGateway ses propres exceptions pour l'instant que c'est très peu de code et appelé uniquement par CmisConnector. Donc en compromis, c'est CMISGateway qui raise directement les APIError.

Ok.

Ack.

commit 830530286e74ffcabe338b5fe5d3801344fb0c88
Author: Emmanuel Cazenave <ecazenave@entrouvert.com>
Date:   Fri Dec 22 18:10:22 2017 +0100

    refactoring of the cmis connector (#20862)

    Restrictions:
     - hanlde only the 'ATOM' protocol
     - use only the 'default' repository of the CMIS server