Update documentation #45

marienfressinaud · 2017-08-23T12:30:24Z

J'ai viré la référence dans la doc à l'option encoding, mais faudrait peut-être aller plus loin. Je voulais lancer le test d'encoding sans justement préciser l'option mais j'arrive pas à installer Ruby 2.2.1 (qu'il faudrait mettre à jour d'ailleurs mais j'arrive pas à installer les dépendances avec la version 2.4.1 donc j'abandonne pour le moment).

jibidus · 2017-08-24T07:58:52Z

Cool une PR !

J'ai aussi une branche Doc en préparation. Je rajouterai quelques commits à ta branche.
Pour l'encodage, j'ai l'impression que c'est un peu plus compliqué (cf doc PostgreSQL), il y a aussi une histoire d'encodage du client PostgreSQL (très probablement défini par le database.yml). Je vais essayer de faire quelques tests pour clarifier tout ça.
Quant à la version de ruby, pourquoi monter de version ? Pour une gem, j'imagine que plus la version est basse, moins il y a de contrainte à l'utilisation. A moins que je ne rate quelque chose ?

jibidus · 2017-08-28T07:16:14Z

Les commits de doc dont je parlais ont déjà été mergés.

Concernant l'encodage, ce paramètre n'est pas négligeable. Exemple avec le test spec/csv_fast_importer_spec.rb:24 qui ne passe plus chez moi si je l'enlève (ma base de donnée est encodée en UTF8).

J'ai regardé un peu plus en détail et l'encodage apparait à plusieurs niveaux :

fichier (File.new)
client postgres (défini par database.yml)
commande COPY (utilise par défaut celui du client postgres. Peut-être redéfini par le paramètre de CSVFastImporter)
base de donnée (chaque base de donnée à associée à un encodage particulier. Cf commande psql -l)

De ce que je comprends, pour que tout ça fonctionne, il faut que la commande COPY utilise le même encodage que celui du fichier, et que cet encodage soit "compatible“ avec celui de la base de donnée.
Qu'en penses-tu ?

jibidus · 2017-09-01T08:12:44Z

J'ai poussé 2 commits (une FAQ avec entrée sur l'encoding et un test pour illustrer la conversion d'encoding avec File::new).
Qu'en penses-tu ?

marienfressinaud

Oui il est top ce fichier ! Il faudrait aussi ajouter un lien depuis le README et faire sauter ma modif qui vire l'option encoding de la doc (ça je peux m'en occuper mais je veux éviter qu'on se marche sur les pieds en bossant sur la même branche)

marienfressinaud · 2017-09-01T13:11:19Z

doc/faq.md

+
+## How to specify encoding?
+
+Multiple componants are involved when `CSV Fast Importer` is executed:


s/componant/component/g

marienfressinaud · 2017-09-01T13:11:45Z

doc/faq.md

+- SQL command (`COPY` for PostgreSQL)
+- database server
+
+Encoding may be consistent accross all these componants. Here is how to specify or check each componant encoding.


"must be" plutôt que "may be", non ?

marienfressinaud · 2017-09-01T13:15:13Z

doc/faq.md

+Ruby `File` can also handle internal and external encoding (see [File::new](https://ruby-doc.org/core-2.4.1/File.html#method-c-new) which can be useful to manage automatic conversion:
+
+```ruby
+File.new 'path/to/file.csv', external_encoding: 'UTF-8', internal_encoding: 'ISO-8859-1'


Je connaissais pas ! On peut aussi abréger en encoding: 'UTF-8:ISO-8859-1' à priori, j'aime bien cette notation. Par contre je pense que tu voulais plutôt représenter l'inverse : external_encoding: 'ISO-8859-1', internal_encoding: 'UTF-8' (soit encoding: 'ISO-8859-1:UTF-8')

marienfressinaud · 2017-09-01T13:20:50Z

Pour ce qui est de la version de Ruby le problème c'est que plus elle est basse est plus il est compliqué de la compiler à cause des dépendances qui ne fonctionnent plus. Après on peut laisser gérer la CI pour tester les différentes versions de Ruby et laisser les devs la possibilité de développer avec une version récente (donc juste changer le fichier .ruby-version).

marienfressinaud · 2017-09-01T13:23:24Z

J'ai vu dans le README que tu avais précisé dans les limitations : "Custom line serparator (ex: \r\n for windows file) is not supported yet". En fait si, il suffit de faire ceci :

file = File.new(path, universal_newline: true)

et hop, magie :) (bon ça marche pas si, par exemple, tu as un mélange de \r et de \n dans ton fichier mais dans ce cas-là c'est qu'il y a eu une couille dans le pâté au moment de l'enregistrement du fichier…)

jibidus · 2017-09-13T06:19:32Z

C'est étonnant ce paramètre universal_newline, je ne le trouve pas dans la doc de IO::new 😮.
Pour la version de ruby, on peut faire ça, en effet. Je le note pour une autre PR.
Je vais modifier la PR par rapport à toutes nos remarques.

jibidus · 2017-09-13T07:26:35Z

J'ai fais les modifs.

Par contre, pour le paramètre magique, je n'arrive pas à reproduire le problème pour vérifier que ça fait bien ce qu'on pense que ça fait :

  describe 'with custom line separator' do
    before do
      file = write_file [ %w(name id), %w(Karadoc 1), %w(Perceval 2) ], row_sep: "\r\n"
      file_with_conversion = File.new file.path      # universal_newline: true
      CsvFastImporter.import file_with_conversion
    end

    it 'imports as many rows as lines in file' do
      row_count.should eql 2
      db.query('SELECT name FROM knights where id = 1').should eql 'Karadoc'
      db.query('SELECT name FROM knights where id = 2').should eql 'Perceval'
    end
  end

Est-ce que tu as une idée @marienfressinaud ?

marienfressinaud · 2017-09-13T07:33:01Z

Tu veux dire que ce test passe sans avoir besoin du paramètre ?

jibidus · 2017-09-13T07:33:44Z

Ouaip

marienfressinaud · 2017-09-13T07:39:10Z

Bizarre... première idée : vérifier que le fichier est bien créé avec les \r\n. Deuxième idée : essayer avec juste \r. Troisième idée : mélanger \r et \r\n au sein d'un même fichier.

jibidus · 2017-09-13T08:20:02Z

Le fichier est bien construit avec les \r\n, d'après la commande file qui me donne ASCII text, with CRLF line terminators (VS uniquement ASCII text avec \n).
Avec juste \r, l'import échoue, même avec universal_newline: true 😢.
Mélanger \r et \r\n, c'est bien sournois.

marienfressinaud · 2017-09-13T08:37:19Z

Bizarre que ça ne marche pas 🤔 pourtant c'est exactement le cas que j'ai et c'est l'option qui m'a sauvé... tu as quoi comme erreur ?

jibidus · 2017-09-13T08:39:35Z

PG::UndefinedColumn:
karadoc" of relation "knights" does not exist

Comme s'il n'identifiait par \r comme un retour à la ligne. Du coup, il est toujours dans l'ent-ête et il cherche une colonne avec le nom \rKaradoc.

Le code du test :

  describe 'with custom line separator' do
    before do
      file = write_file [ %w(name id), %w(Karadoc 1), %w(Perceval 2) ], row_sep: "\r"
      puts `file #{file.path}`
      file_with_conversion = File.new file.path, universal_newline: true
      CsvFastImporter.import file_with_conversion
    end

    it 'imports as many rows as lines in file' do
      row_count.should eql 2
      db.query('SELECT name FROM knights where id = 1').should eql 'Karadoc'
      db.query('SELECT name FROM knights where id = 2').should eql 'Perceval'
    end
  end

marienfressinaud · 2017-09-13T08:42:36Z

ok, je vois rien qui cloche a priori, j'essaierai de regarder dans la journée si j'ai le temps

marienfressinaud · 2017-09-18T09:24:07Z

J'ai regardé et… c'est un vrai mystère pour moi ! Concernant la doc de universal_newline c'est en fait indiqué mais de façon très indirecte :

https://ruby-doc.org/core-2.2.0/File.html#method-c-new : See IO.new for a description of mode and opt.
https://ruby-doc.org/core-2.2.0/IO.html#method-c-new : Also, opt can have same keys in String#encode for controlling conversion between the external encoding and the internal encoding. (en dessous de la liste des options)
https://ruby-doc.org/core-2.2.0/String.html#method-i-encode : :universal_newline Replaces CRLF (“rn”) and CR (“r”) with LF (“n”) if value is true.

Ouf !

Ceci dit, ça n'aide pas à faire fonctionner… j'ai pensé que potentiellement vu que le fichier avait été ouvert, Ruby le gardait potentiellement en mémoire et ne s'embêtait pas à le réouvrir à l'appel de File.new, mais même en ne faisant qu'ouvrir un fichier déjà existant, ça ne marche pas.

J'ai ensuite testé avec une version plus récente de Ruby (un bug corrigé dans une version plus récente ?) mais non, ça n'est pas mieux.

J'ai quand même vérifié de mon côté que ça marchait sur mon-assistant et oui, j'ai bien un fichier avec des \r et j'arrive à l'importer avec l'option universal_newline… il y a vraiment quelque chose qui m'échappe.

marienfressinaud · 2019-08-13T09:10:00Z

Hey coucou ! :) Je suis en train de faire le ménage dans mes PR ouvertes, et celle-là date d'il y a bientôt 2 ans 🎉

J'ai pas très envie de la fermer mais j'ai pas d'énergie à passer dessus. D'après les derniers commentaires, il y avait seulement une option qui nous embêtait donc je propose :

soit de laisser tomber cette option
soit de la documenter en précisant "on n'arrive pas à la tester donc c'est pas sûr que ça marche correctement"

jibidus · 2019-08-14T06:26:01Z

Hello !
Oui, je l'ai toujours dans ma todo list, mais pas le temps / courage depuis ...
Bref, le commentaire le semble pas mal avec une ref sur cette pr.
Je caresse l'espoir de m'y remettre un jour...

marienfressinaud · 2019-12-06T14:58:50Z

On merge ? :)

marienfressinaud commented Sep 1, 2017

View reviewed changes

jibidus force-pushed the tec/documentation branch from 38ec6aa to 11a87f3 Compare September 13, 2017 07:24

jibidus force-pushed the tec/documentation branch from 11a87f3 to be156ba Compare September 13, 2017 07:32

jibidus force-pushed the tec/documentation branch from 2ee1de6 to d0aec0e Compare September 13, 2017 07:53

Marien Fressinaud and others added 3 commits September 13, 2017 10:10

Doc / Fix errors in README

ee54b91

Doc / FAQ about encoding

6ec4349

Chore / Test file encoding conversion

fe88e87

jibidus force-pushed the tec/documentation branch from d0aec0e to fe88e87 Compare September 13, 2017 08:11

Doc / Fix typo and add notes (#54)

0b4d232

jibidus approved these changes Dec 6, 2019

View reviewed changes

jibidus merged commit 04993ef into master Dec 6, 2019

jibidus deleted the tec/documentation branch December 6, 2019 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update documentation #45

Update documentation #45

marienfressinaud commented Aug 23, 2017

jibidus commented Aug 24, 2017

jibidus commented Aug 28, 2017

jibidus commented Sep 1, 2017 •

edited

marienfressinaud left a comment

marienfressinaud Sep 1, 2017

marienfressinaud Sep 1, 2017

marienfressinaud Sep 1, 2017

marienfressinaud commented Sep 1, 2017

marienfressinaud commented Sep 1, 2017

jibidus commented Sep 13, 2017

jibidus commented Sep 13, 2017

marienfressinaud commented Sep 13, 2017

jibidus commented Sep 13, 2017

marienfressinaud commented Sep 13, 2017

jibidus commented Sep 13, 2017 •

edited

marienfressinaud commented Sep 13, 2017

jibidus commented Sep 13, 2017 •

edited

marienfressinaud commented Sep 13, 2017

marienfressinaud commented Sep 18, 2017

marienfressinaud commented Aug 13, 2019

jibidus commented Aug 14, 2019

marienfressinaud commented Dec 6, 2019


		## How to specify encoding?

		Multiple componants are involved when `CSV Fast Importer` is executed:

Update documentation #45

Update documentation #45

Conversation

marienfressinaud commented Aug 23, 2017

jibidus commented Aug 24, 2017

jibidus commented Aug 28, 2017

jibidus commented Sep 1, 2017 • edited

marienfressinaud left a comment

Choose a reason for hiding this comment

marienfressinaud Sep 1, 2017

Choose a reason for hiding this comment

marienfressinaud Sep 1, 2017

Choose a reason for hiding this comment

marienfressinaud Sep 1, 2017

Choose a reason for hiding this comment

marienfressinaud commented Sep 1, 2017

marienfressinaud commented Sep 1, 2017

jibidus commented Sep 13, 2017

jibidus commented Sep 13, 2017

marienfressinaud commented Sep 13, 2017

jibidus commented Sep 13, 2017

marienfressinaud commented Sep 13, 2017

jibidus commented Sep 13, 2017 • edited

marienfressinaud commented Sep 13, 2017

jibidus commented Sep 13, 2017 • edited

marienfressinaud commented Sep 13, 2017

marienfressinaud commented Sep 18, 2017

marienfressinaud commented Aug 13, 2019

jibidus commented Aug 14, 2019

marienfressinaud commented Dec 6, 2019

jibidus commented Sep 1, 2017 •

edited

jibidus commented Sep 13, 2017 •

edited

jibidus commented Sep 13, 2017 •

edited