On Tue, 29 Nov 2011 18:17:21 +0100, Brendan Le Ny <bleny@codelutin.com> wrote:
Le 29/11/2011 15:02, desbois a écrit :
- 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.
Ok, je verrais en creusant le ModelBuilder.
- 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)
mandatory = true, ignored = false
- Optionnelle (la colonne peut être présente et elle sera traitée)
mandatory = false, ignored = false
- Ignorée (la colonne peut être présente et elle ne sera pas traitée)
mandatory = false, ignored = true
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.
A voir dans Column si c'est pas une erreur d'avoir mandatory = !ignored du coup.
- 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.
Oui en relisant le contenu de la méthode j'ai vu qu'elle placait juste le curseur sur la première ligne. A faire dans la méthode iterator() du coup.
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).
Je garderais le readHeaders qui est une notion importante à garder en dehors. Pour le close, ya quand meme un CsvReader.close() derrière. On sait jamais, ya ptet plus que la fermeture du flux. Mieux vaut avoir une vrai méthode close().
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.
Ouais je vois, on verra au besoin.
Merci pour toutes ces remarques constructives et pour ta réactivité :-)
Avec plaisir ;)