Le 29/11/2011 15:02, desbois a écrit :
J'ai jeté un oeil sur nuiton-csv, et je vais en profiter pour rajouter des tests unitaires et de la doc.
Cool. Mes commentaires à prendre avec de légères pincettes étant donné que je me base sur ce que j'ai fait dans Wao, j'ai pas eu le temps de voir les modifs de Tony après son extraction vers nuiton-utils.
J'ai pour le moment juste tester l'Import.
C'est le plus sensible (l'export pose moins de soucis).
Quelques retours jusque là :
- Je ne suis pas fan du Iterator anonyme au milieu de la méthode startImport. Je pense qu'il serait plus intéressant de le sortir en méthode iterator() et du coup faire que Import implémente Iterable.
Ouais, moi non plus. Je pense que Iterable est une très bonne idée. et du coup le startImport devient @Override iterator();
- Ma première utilisation de Import fut de l'utiliser tel quel en implémentant les ImportableColumn. Je me suis aperçu qu'il y avait une implémentation par défaut très intéressante qui est Column. Du coup je me pose la question de l'interface. A priori les méthodes static proposés suffisent pour instancier correctement le Column suivant le besoin (import, export ou les deux). Je simplifierais donc bien en utilisant tout le temps Column. (cela enlèverait donc trois interfaces).
Ouais, à voir, les contrats sont bien (ils réifient vraiment une notion, un concept de l'API). Par contre l'implém (Column + Builder) est seulement une solution pour fournir des implémentations de ces interfaces et pour moi, cette partie de l'API est carrément améliorable. L'interface du builder pourrait être plus flexible.
- La solution de mandatory/ignored est un peu confuse. L'implémentation dans Column suppose que mandatory = !ignored, ce qui finalement se tient. On aura jamais les deux à true ou les deux à false. A moins que j'ai zappé un cas, je pense qu'il faut supprimer l'un des deux.
Ouais, tu zappes un cas. Y'a bien trois cas : - Obligatoire (la colonne doit être présente et elle sera traitée) - Optionnelle (la colonne peut être présente et elle sera traitée) - Ignorée (la colonne peut être présente et elle ne sera pas traitée) La quatrième cas par contre me semble inutile parce qu'il me semble aberrant d'exiger la présence d'une donnée qui ne sera pas traitée. Certains vont se demander « Pourquoi déclarer une colonne dans le modèle si on ne fait pas de traitement » -> tout simplement pour qu'à la validation du CSV on vérifie bien que tous les en-têtes présents sont connus.
- La notion de readFirstLine pourrait être paramètrable via le modèle. A moins que ce soit un pré-requis obligatoire que la première ligne soit les HEADERS. (Ce qui sera ajouter explicitement à la doc)
Perso, ça me paraît étrange d'avoir les entêtes plus loin que la première ligne. Par ailleurs, cette méthode ne doit pas être appelée par l'utilisateur, c'est une méthode interne pour simplement commencer l'itération en sautant la ligne des headers et à bien commencer la numérotation des lignes à 1.
- J'ai du mal avec la notion de start/stop. Je garderais plutôt l'idée de read/close plus explicite sur ce qui se cache derrière (un fichier).
Faut oublier, start/stop, c'est une erreur de jeunesse de ma part. Comme dit plus, haut, le start() devient iterator(); et le stop n'a pas lieu d'être. Il ne sert qu'à fermé l'InputStream (le csv à lire) or il faut toujours laisser le soin à l'appelant de le faire de toute façon donc pour moi ça doit dégager de l'API.
L'iterator se chargera du parcours. A noter que le validateCsv ne valide que les headers et non l'intégralité des données (ce qui serait débile mais peut porter à confusion).
Exact, cela vérifie : * qu'aucun entête lu dans le CSV est inconnu du modèle * que tous les champs obligatoires d'après le modèle sont présent * que le modèle est cohérent (pas de colonnes déclarées en doubles)
Utilisation actuelle :
Import<Row> importRow = Import.newImport(rowModel, stream); try {
Iterator<Row> iterator = importRow.startImport(); // validate headers, read first line and return iterator
while (iterator.hasNext()) { Row row = iterator.next();
// treatment }
} finally {
importRow.stopImport(); }
Utilisation possible :
Import<Row> importRow = Import.newImport(rowModel, stream); try {
importRow.readHeaders(); // validate headers and read first line
for (Row row : importRow) { // using importRow as Iterable
// treatment }
} finally {
importRow.close(); }
Oui, sans le importRow.readHeaders(); qui est interne normalement à Import. et dans le finally, c'est plutôt un IOUtils.close(stream).
Voilà pour le moment, je n'ai pas encore regarder le ModelBuilder. Je vais commiter mon test d'Import (avec Model et ImportableColumn anonymes, avec Model et Column)
Une autre idée en passant : Est-ce que ce serait pas plus intéressant d'avoir le choix entre un rapport d'erreurs ou des exceptions dans le cas d'un problème rencontré sur une ligne ? A l'heure actuelle il faut catcher les exceptions une par une pour toutes les avoirs. Ce sera probablement intéressant dans un deuxième temps si le besoin s'en fait sentir.
C'est compliqué, perso, je me suis un peu cassé la tête avec ça parce que le contrat Iterator ne laisse pas passer les exeptions. Du coup, toute exception déclenchée par le modèle, au moment de la lecture d'une ligne est encapsulée dans une RuntimeException, throwée par le next() puis désencapsulée derrière, du coup l'API est pas très représentative de ce qui peut arriver :-( J'aime pas du tout cet aspect de l'API. Merci pour toutes ces remarques constructives et pour ta réactivité :-) -- Brendan Le Ny <bleny@codelutin.com> Code Lutin Conseil & Développement Logiciel Libre +33 (0)2 40 50 29 28 http://codelutin.com