Conversation
ghost
self-assigned this
ghost
force-pushed
the
simplify-backends
branch
from
1663895 to
48264df
Compare
thib-stpl
left a comment
thib-stpl
left a comment
There was a problem hiding this comment.
Je ne me prononce pas sur la simplification de l'auto-détection des backends. Je ne me rends pas compte à quel point c'est lié à la modification sur les options du backend CSV.
Pour les options en tout cas cela m'arrange, et j'ai pu valider que j'avais le comportement attendu en utilisant cette branche.
| @csv = CSV.new(io, **CSV_OPTS) | ||
| @headers = detect_headers(@csv) | ||
| @cols_count = @headers.size | ||
| end |
There was a problem hiding this comment.
On obtient maintenant les options CSV depuis une méthode plutôt qu'une constante, c'est plus pratique.
Vu que c'est une méthode « de classe », cela nous oblige toujours à surcharger #initialize sauf erreur de ma part. En tout cas si on veut définir cette option dynamiquement en fonction de l'IO, qui n'est accessible que lorsque la classe est instanciée.
Cela permet d'utiliser super et simplifier un peu :
https://github.com/steeple-org/obelix/commit/416619d3ba94110dc02a018210e20ecaa91ff49d#diff-1fbd58936604235acab1b8ddf9ec5b3f038276a1c14e8ede2c2fcddfac03cf1b
thib-stpl
left a comment
thib-stpl
left a comment
There was a problem hiding this comment.
Supprimer/extraire la modification sur le codage de caractères.
lib/sheetah/backends/csv.rb
Outdated
| col_sep: self.class.defaults[:col_sep], | ||
| quote_char: self.class.defaults[:quote_char] | ||
| ) | ||
| ensure_utf8(io) |
There was a problem hiding this comment.
Si possible, supprimer/extraire cette modification sur le codage de caractère.
Par contre après sur la mise à jour correcte de la gem je rencontre un petit soucis, rapport à la modification sur le codage de l'IO :
https://github.com/steeple-org/obelix/commit/524e250aebd41d9622c23e6da48faed774c23708
Notre internal_encoding est nil dans ce cas, et je suis pas convaincu que ce soit une bonne idée de le forcer. external_encoding ne correspond pas non plus dans ce cas (ASCII-8bit).Donc je vais attendre que tu extrais ou supprimes cette partie là pour me remettre dessus.
The CSV backend should not have an opinion on the encoding(s) of the IO object it is given. The IO is supposed to be configured with consistent encodings, and that's it.
ghost
force-pushed
the
simplify-backends
branch
from
48264df to
b18c834
Compare
ghost
marked this pull request as ready for review
1 participant
The general idea is to avoid being clever with backends, and prefer explicit customization by users.