Refactor - split into Reader and Writer classes #27

odinuv · 2018-04-23T13:17:24Z

No description provided.

tests: use temp directory for writer

tomasfejfar · 2018-04-23T14:17:01Z

src/CsvReader.php

    {
        rewind($this->getFilePointer());
        $sample = fread($this->getFilePointer(), 10000);
-        rewind($this->getFilePointer());


Nespoléhal bych na to, že to nikdo nezavolá jen proto, že je to protected. Ta třída jde extendovat a budou z toho těžko odhalitelné chyby. Nebo co ji dát final?

tomu nerozumim, vadi ti, ze ma detectLineBreak side effect? no a co? nejmenuje se to getLineBreak (srovnej se soucasnou verzi, kde getLineBreak ma sideeffect), stejne tak ma openCsvfile a closeCsvFile maji side effect, pokud si tu tridu nekdo extenduje a zavola to nekde uprostred cteni/zapisu, tak je proste blbej

nevidim smysl v tom snazit se tomu nejak zabranit

tomasfejfar · 2018-04-23T14:19:58Z

src/CsvReader.php

            );
        }
+        $this->lineBreak = $this->detectLineBreak();
+        rewind($this->getFilePointer());


proč je tady jiný rewind než na L176?

tj. proč tady není $this->rewind()

protoze to dela neco jinyho - header je vzdycky prvni radek, ale $this->rewind respektuje skip lines, kdyby se tady volalo $this->rewind, tak bude header nejaka blbost

tomasfejfar · 2018-04-23T14:20:52Z

src/CsvReader.php

+            return $this->header;
        }
-
        return [];


Kdy to propadne sem? IMHO nikdy, ne?

u prazdnyho souboru

tomasfejfar · 2018-04-23T14:23:49Z

src/CsvWriter.php

+        $return = [];
+        foreach ($row as $column) {
+            if (!(
+                is_scalar($column)


Tady mě ještě teď když to vidím zformátované napadlo - nešlo by z toho udělat if (!$stringable) - to by neovlivnilo early exit v tom boolean výrazu a přitom by byla ta podmínka pak čitelnější.

$isStringable
Diff 0 : 489.37952709198
Diff 1 : 497.38059091568
Diff 2 : 503.51376700401
Diff 3 : 505.61457920074
Diff 4 : 507.42060494423
Diff 5 : 504.46329498291
Diff 6 : 503.64453482628
Diff 7 : 501.33763980865
Diff 8 : 501.47956895828
Diff 9 : 497.10404992104

vs

soucasny stav
Diff 0 : 474.2037358284
Diff 1 : 463.99877309799
Diff 2 : 459.16944909096
Diff 3 : 457.29524803162
Diff 4 : 472.84506487846
Diff 5 : 491.63278388977
Diff 6 : 492.76865696907
Diff 7 : 489.76580095291
Diff 8 : 488.95872497559
Diff 9 : 478.8238589763

sice uz to neni takovej rozdil, ale je tam, takze ne, nebudu to prepisovat aby to bylo citelnejsi :) budes se muset naucit cist

tomasfejfar · 2018-04-23T14:25:30Z

src/CsvWriter.php

+    /**
+     * @return resource
+     */
+    protected function getFilePointer()


Ta metoda je stejná pro reader i writer, ne? Tak by mohla být v tom abstraktním předkovi.

je, a puvodne jsem ji tam mel, ale je to na palici, protoze ten pointer ma jinou inicializaci, takze zusatane v abstraktni tride nesmyslnej fragment, a vlastne je teda metoda stejna jen "shodou okolnosti"

tak nakonec jsem to udelal :) pridanim tech pointeru tam vzniklo vic spolecnyho kodu a zda se , ze to vyresila jedna abstraktni metoda

tomasfejfar · 2018-04-23T14:33:24Z

tests/CsvReadTest.php

+    {
+        try {
+            $csv = new CsvReader(__DIR__ . '/nonexistent.csv');
+            $csv->getHeader();


Nevyhodí to exception i bez toho načtení headeru teď?

copy & paste :( bohuzel to tam bylo uz z predchoziho pr :(

fixed

tomasfejfar · 2018-04-23T14:35:40Z

tests/CsvReadTest.php

+        $fileName = __DIR__ . '/data/simple.csv';
+
+        $csvFile = new CsvReader($fileName);
+        self::assertEquals("\n", $csvFile->getLineBreak());


Nechybí tady ještě nějaké ověření načtení jako v testu nad tím? A neměli by se jmenovat stejně? buď "...WithoutRewind" nebo "...NoReset"

tomasfejfar · 2018-04-23T14:41:23Z

tests/CsvReadTest.php

+
+        $writer->writeRow($rows[0]);
+        $reader->next();
+        self::assertEquals(false, $reader->current());


jakto? Neměl by tam být už ten zapsaný řádek?

Bylo by fajn tam dát komentář nebo message k tomu assertu

nemel, protoze ten reader je nakonci, musi se mu tam dat rewind
je to counterintuitive a nereprezentuje to zadnej usecase, ale je to dokumentace toho jak se to chova pokud udelas takovyhle zverstvo :) Nicmene porad je to lepsi nez stavajici verze, ktera je schopna si prepsat existujici radky (protoze ma stejny pointer pro read i write)

tomasfejfar · 2018-04-23T14:44:41Z

tests/CsvWriteTest.php

+    {
+        $csvFile = new CsvWriter(sys_get_temp_dir() . '/test-write.csv');
+        self::assertEquals('test-write.csv', $csvFile->getBasename());
+        self::assertEquals("\"", $csvFile->getEnclosure());


replace quotes " → '

tomasfejfar · 2018-04-23T14:50:22Z

tests/CsvWriteTest.php

+                "columns with\nnew line", "columns with\ttab",
+            ],
+            [
+                'column with \n \t \\\\', 'second col',


je tady ' schválně? Takhle to je skutečně \n a ne nový řádek

Jo, koukám, že podle testu je to asi expected...

ano, je, testuje to, ze se to neinterpretuje

Halama

Komenty níže. Plus jsem si ještě všimnul že nejsou updatnuté examples v Readme.

Halama · 2018-04-27T06:09:25Z

src/CsvWriter.php

+    /**
+     * @throws Exception
+     */
+    protected function openCsvFile()


Dal bych vše v reader i writer jako private a ideálně by ty classy mohly být final jak myslím už zmiňoval Tomáš.

Halama · 2018-04-27T06:17:34Z

src/AbstractCsvFile.php

+
+use SplFileInfo;
+
+class AbstractCsvFile extends SplFileInfo


Když už se to refaktoruje tak si myslím že by to němělo dědit ten SplFileInfo, ale měl by být jako private property. To by mohlo umožnit např. založit ten reader/writer už z existujícího file resource. Na což jsem párkrát narazil a jsou na to i issues #14 a #5

odinuv · 2018-04-27T07:07:16Z

Jeste me napadlo, jestli nevyhodit jeden z tech exception kodu - je tam string code a number code https://github.com/keboola/php-csv/blob/master/src/Exception.php#L7

…y used

tests: add check for header with skip lines

feat: allow working with file resources directly

tomasfejfar · 2018-05-02T11:34:54Z

src/CsvReader.php

+    /**
+     * @var string
+     */
+    private $fileName;


Nechceš si nastavit oneline field docy?

ja ti nevim, me se to nejak nelibi :) premyslel jsem nad tim proc a asi mam dva duvody, jednak k fieldum nekdy pisu komentar k cemu jsou a jednak me drazdi kdyz je phpdoc u metody jinej nez u fieldu :)

Motivace proč to používat je veskrze:

fieldy jsou na začátku třídy a takhle máš 1(blank)+3(doc)+1(def) x N místo 1+1+1 x N řádků. Tj. prostě víc scrollovaní

fieldy většinou (!) nemívají description a ostatní součásti docblocku (a pokud ano, tak ho můžeš udělat víceřádkový

Ale více méně souhlasím, že i tvoje argumenty jsou validní ;)

odinuv · 2018-05-02T11:36:08Z

jeste me napadlo, ze vlasne muzu vyhodit ten mode, kdyz je tam pointer

Halama · 2018-05-02T11:54:20Z

src/CsvWriter.php

+    protected function closeFile()
+    {
+        if (is_resource($this->filePointer)) {
+            fclose($this->filePointer);


Nemělo by ho to zavřít jen když ho to samo otevřelo? Vím že nějaká knihovna se takhle chovala že to vždy zavírala a pěkně mě to štvalo.

a to samé pro reader.

nj, to me uplne uslo!

Halama

Myslím že by to nemělo zavírat resources které jsou tomu předány zvenčí.

…-reader-writer

odinuv · 2018-05-13T07:42:31Z

@tomasfejfar @Halama merge?

Halama · 2018-05-13T09:07:33Z

nějak se mi nezdá tohle co jsi nenápadně propašoval do masteru:) Když error_get_last vrání null tak to failne.

Halama · 2018-05-13T09:09:37Z

src/CsvWriter.php

+        if (($ret === false) || (($ret === 0) && (strlen($str) > 0))) {
+            throw new Exception(
+                "Cannot write to CSV file " . $this->fileName .
+                ' Error: ' . error_get_last()['message'] . ' Return: ' . json_encode($ret) .


Myslím že se může stát že v error_get_last nic nebude a pak to failne při přístupu k null.

aha, no to mi uslo, ze error_get_last vraci null, protoze jsem to zkousel bez erroru. mazec:

php > var_export(null['test']); php > NULL

takze error to neudela, ale vrchol cistoty to teda neni :) tak tam asi dam json_encode

Zajímavý to s tím null jsem neznal :)

Accessing variables of other types (not including arrays or objects implementing the appropriate interfaces) using [] or {} silently returns NULL.

http://php.net/manual/en/language.types.string.php
Jsem se tam dostal přes tohle https://stackoverflow.com/questions/10990321/why-does-php-not-complain-when-i-treat-a-null-value-as-an-array-like-this

Tohle je taky super zápis $z = null['neco']['sdfdsf']; :)

… any tests: added tests for different types of errors

Halama · 2018-05-14T07:00:47Z

hraju si s tím, moc netuším proč ten fopen háže tu chybu https://travis-ci.org/keboola/php-csv/jobs/378425803#L574

odinuv · 2018-05-14T07:01:31Z

no zrovna na to taky koukam jak tele na novy vrata

Halama · 2018-05-14T07:02:28Z

aha on ten error_get_last tam bude z jineho testu:( Tohle projde php vendor/bin/phpunit --debug --filter testInvalidPointer

Halama · 2018-05-14T07:05:55Z

bych tam dal na začátek writeRow error_clear_last();

Halama · 2018-05-14T07:08:56Z

nebo to volat jenom když to vrátí false předtím

Halama · 2018-05-14T07:10:09Z

public function writeRow(array $row)
    {
        $str = $this->rowToStr($row);
        $ret = @fwrite($this->getFilePointer(), $str);

        /* According to http://php.net/fwrite the fwrite() function
         should return false on error. However not writing the full
         string (which may occur e.g. when disk is full) is not considered
         as an error. Therefore both conditions are necessary. */
        if (($ret === false) || (($ret === 0) && (strlen($str) > 0))) {
            throw new Exception(
                "Cannot write to CSV file " . $this->fileName .
                ($ret === false ? 'Error: ' . error_get_last()['message'] : '') .
                ' Return: ' . json_encode($ret) .
                ' To write: ' . strlen($str) . ' Written: ' . $ret,
                Exception::WRITE_ERROR
            );
        }
    }

odinuv · 2018-05-14T07:11:09Z

uz asi vim

odinuv · 2018-05-14T07:11:49Z

aha, zase se me neupdatoval github!, sakra - jo presne tak

Halama · 2018-05-14T07:18:18Z

Myslím že bysme ještě do tests/bootstrap.php měli přidat error_reporting(-1); https://github.com/aws/aws-sdk-php/blob/5177cbe73c96d25ff759e406ac81b8e25557482c/tests/bootstrap.php#L2
Teď to nefailne na notice apod. Zkoumal jsem to chování null...

Halama

Za mě ok.

Halama · 2018-05-14T07:41:58Z

Dotlačíš prosím ty fixy error handlingu i do 1.4.x ? Hodil bych to pak do toho Redshift extractoru.

…-reader-writer

odinuv · 2018-05-14T08:20:35Z

done

tomasfejfar · 2018-05-15T09:46:53Z

Jen ideálně za mě nemergovat master to branche, ale rebasovat branch nad master ;)

Halama · 2018-05-18T09:00:09Z

Releasnu to

odinuv · 2018-05-18T09:05:55Z

jo

odinuv added 6 commits April 16, 2018 10:18

refactor: reorganize tests into reader and writer tests

5a25d00

refactor: reorganize tests into reader and writer tests

fb574c8

refactor: reorganize tests into reader and writer tests

9018818

refactor: split into reader and writer classes

68ab0a6

refactor: split into CSVReader and CSVWriter

ca998d7

refactor: remove lazy initialization

19e0c81

tests: use temp directory for writer

odinuv requested review from Halama and tomasfejfar April 23, 2018 13:17

odinuv added 2 commits April 23, 2018 15:27

tests: fix invalid line break test

1f1589c

chore: cs fixes

92f45cb

tomasfejfar requested changes Apr 23, 2018

View reviewed changes

odinuv added 5 commits April 26, 2018 21:56

fix: remove + modes

fe2a040

fix: use correct defaults

3c5325a

tests: polished

2161707

tests: polished

4179ac8

tests: polished

ca0e242

Halama reviewed Apr 27, 2018

View reviewed changes

odinuv added 7 commits April 28, 2018 23:31

fix: use correct exception params

17fc0cc

refactor: remove exception context params, because they are not reall…

fda01c0

…y used

chore: remove unused variable

92e590f

docs: update

9a107c5

tests: add check for header with skip lines

refactor: remove dependency on splfileinfo

9d5ca09

feat: allow working with file resources directly

refactor: make fields private

9387be9

feat: no hardcoded line break

7325572

tomasfejfar approved these changes May 2, 2018

View reviewed changes

tomasfejfar reviewed May 2, 2018

View reviewed changes

Halama reviewed May 2, 2018

View reviewed changes

Halama requested changes May 2, 2018

View reviewed changes

Merge branch 'master' of https://github.com/keboola/php-csv into odin…

eb55800

…-reader-writer

Halama reviewed May 13, 2018

View reviewed changes

fix: better error message on failed fwrite, also don't spit notice if…

572371a

… any tests: added tests for different types of errors

odinuv added 2 commits May 14, 2018 09:14

fix: more robust error checking

a101d2f

tests: portable error message

44a4914

odinuv added 2 commits May 14, 2018 09:23

tests: more portable error message

bb673a8

tests: catch notices

0916f78

Halama approved these changes May 14, 2018

View reviewed changes

odinuv added a commit that referenced this pull request May 14, 2018

fix: error reporting from #27

eb77f51

Merge branch 'master' of https://github.com/keboola/php-csv into odin…

3bc11a7

…-reader-writer

tomasfejfar approved these changes May 15, 2018

View reviewed changes

Halama mentioned this pull request May 18, 2018

#29 Enable MacOS Line breaks as acceptable #30

Open

Halama merged commit d46e83e into master May 18, 2018

Halama deleted the odin-reader-writer branch May 18, 2018 09:12

Refactor - split into Reader and Writer classes #27

Refactor - split into Reader and Writer classes #27

Uh oh!

Conversation

odinuv commented Apr 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Halama left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odinuv commented Apr 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odinuv May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odinuv commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

odinuv May 4, 2018 •

edited

Loading

Halama May 14, 2018 •

edited

Loading