Produits Entr'ouvert » Passerelle

Ça serait plus clair si l'opérateur utilisé pour gt était > et non < :

def gt_operator(i, v, case_insensitive=False):
    return lambda x: int(v) < int(x[i])

Et peut-être ce serait plus clair si les deux opérandes étaient de même nature, et pas d'un côté l'index dans la ligne et de l'autre la valeur de comparaison ? La valeur tirée de la ligne étant alors tirée avant, dans le models.py.

~~~

Ajouter "icontains" ? Et "ieq" et "nieq" ? Comme ça il n'y aurait pas un "case_insensitive" trimballé partout mais juste la compat :

if query and filters:
    if case_insensitive:
        filters = {'text__icontains': query}
    else:
        filters = {'text__contains': query}

(à propos de ces lignes, dans le patch il y a un coup if query and not filters et puis un coup if query and filters qui fait proche la même chose)

~~~

Mais avant ça, plus généralement, je virerais query et case_insensitive de la signature du get_data(), je ferais plutôt que la vue crée les filtres adéquats.

~~~

import csvoperators : le mettre en tête de module, pas de raison à l'importer seulement là. (et plutôt from . import csvoperators).

~~~

Ce changement n'a pas de rapport, autre ticket autre commit :

-        dialect = csv.Sniffer().sniff(content[:1024])
+       dialect = csv.Sniffer().sniff(content)

~~~

Vraiment un truc qu'il est utile de logguer ? (on a déjà l'URL, qui nous permet de tout déduire, il me semble).

            self.logger.info(filters)

~~~

Je reste vieux et peu à l'aise avec ces yield et itertools et générateurs, etc. Ça se ferait d'ajouter quelques commentaires, genre "# get matches applying all filters on rows" ?

~~~

Ça a déjà été bien pire mais il reste des endroits qui ne sont pas corrects par rapport à l'utilisation des espaces, genre for key,value in filters.items(): (et c'est peut-être le seul, je n'ai pas fait d'inspection détaillée)

0001-csv-datasource-add-POST-method-support.patch
0004-csv-data-source-add-post-method-support-for-tests-11.patch

→ #11059

0002-csv-data-source-add-advanced-filters-support-11057.patch
0005-csv-data-source-makes-view-generates-right-filters-1.patch

devraient être un seul patch, ça facilitera la relecture.

Je pense que nie_operator devrait être ine_operator pour garder la logique de nommage (ne pourrait-on avoir igt, ilt, ige, ile au passage ?), à corriger aussi dans le template detail.

Je le proposais juste par cohérence, la source CSV étant destinée à un usage assez large (entendre par des fonctionnels en autonomie) je pense qu'il faut viser du générique un minimum (et c'est 8 lignes de code).

Et donc discussion par Jabber:

josue mar. 07 juin 2016, 14:04:10
par rapport a https://dev.entrouvert.org/issues/11057. L'idée de faire du igt, ige, ile, ilt pour des str me dérange un peu
enfin, sauf si l'on veut faire de la comparaison de valeurs ASCII 
josue mar. 07 juin 2016, 14:09:49
s/str/char/
bdauvergne@im.libre-entreprise.com mar. 07 juin 2016, 14:10:50
ça de toute façon oui il faut gérer les règles de collation
on s'en sort pas sinon
josue mar. 07 juin 2016, 14:13:32
ok 
bdauvergne@im.libre-entreprise.com mar. 07 juin 2016, 14:15:44
In [9]: locale.setlocale(locale.LC_ALL, 'fr_FR.UTF-8')
Out[9]: 'fr_FR.UTF-8'

In [10]: for x in sorted(l, cmp=locale.strcoll):
    print x
   ....:     
à
ä
e
é
è
l = [u'é', u'à', u'è', u'e', u'ä']
le truc chiant c'est le setlocale
Django a un traitement interne des locales et c'est une action globale

Mon avis c'est de ne pas gérer les règles de collation tout de suite mais dans les opérateurs de comparaison remplacer les < et > par une fonction compare_string(left, right) définie par compare_string = cmp, et donc on remplace left < right par compare_string(left, right) < 0 et on pourra voir plus tard comment on veut traiter les locales (avec locale.setlocale() et locale.strcoll(), ou pyIcu ou je ne sais quoi).

En dehors de la lecture du patch, mon avis, parce que je comprends pas l'idée des "lt/gt" pour les strings. Pour moi ça lt/gt n'ont de sens que pour les nombres, et pour des filtres sur un CSV, ça ne sera disponible que pour des nombres. En effet, on ne connait pas le type des colonnes du CSV, et quand il y aura un filtre "foo_lt=4", on doit considèrer que le valeurs de foo sont entières (à la rigueur décimales si foo_lt=4.1). Voilà c'était mon avis du soir.

Si la valeur est 0, la fonction is_int va retourner False dans le contexte booleen et la comparaison en tant qu'entiers ne se fera pas.
Je verrais plutôt:

def is_int(value):
    try:
         v = int(value)
         return True
    except(ValueError, TypeError):
         return False

modulo l'espace manquant,

except (ValueError, TypeError)
      ^

Aussi,

def test_unknown_operator(client, setup):
[...]
    assert result['err'] == 1
    assert result['err_class'] == 'AttributeError'

Ce serait bien de fournir une info plus précise.

Go