Add loading input state file #48

Actimel · 2018-10-14T08:31:43Z

Solve #46

Actimel · 2018-10-14T08:35:06Z

Nastrelil jsem zatim getter na state file.

IMHO by stalo za to dodelat tam i metodu na zapis do .../out/state.json (#47), co vy na to?

odinuv · 2018-10-15T08:04:02Z

src/BaseComponent.php

+    protected function writeOutputStateToFile(array $state): void
+    {
+        $outDataDir = $this->getDataDir() . '/out';
+        if (!is_dir($outDataDir)) {


pryc s tim, adresar existuje

V testech by nemusel, teoreticky. Spíš co použít SF Filesystem?

Tam by mělo stačit $fs->dump($cesta, $obsah)

no tak snad nemusime psat kod kvuli tomu, aby prochazely testy, ktery neodpovidaji realite :) fs->dump beru, ale stejne tak beru ze to vyleti s unhandled exception, protoze to proste je neocekavana situace

Ok, vyhodim to, u testu problem nevidim, jednoduse tam tu slozku budeme mit uz udelanou.

odinuv · 2018-10-15T08:07:06Z

tests/BaseComponentTest.php

+        $logger = new Logger();
+        $baseComponent = new BaseComponent($logger);
+
+        $this->assertEmpty($baseComponent->getInputState());


assertSame([] ?

Je potreba byt az tak paranoidni? getInputState ma return type array a i kdyz ho nejakym zpusobem zmenis, tak se to vysype jinde.

ne to neni kvuli paranoi, ale kvuli specifikaci. tj.

Pokud soubor neexistuje, tak state je prazdne pole

tak testujme specifikaci ^ a ne neco jinyho.

odinuv · 2018-10-15T08:26:02Z

src/BaseComponent.php

+            $jsonEncoder = new JsonEncoder();
+            $this->inputState = $jsonEncoder->decode($jsonContents, JsonEncoder::FORMAT);
+        } else {
+            $this->inputState = [];


tady je jeden potencionalni problem, kdyz bude ve state {} nebo [] a prozene se to pres assoc decoder, takze z toho vzdycky vznikne []. Vzhledem k tomu, ze state jsou interni data jedne aplikace a ta aplikace je vzdycky v PHP, tak si myslim, ze to vubec nevadi. Presto jsem to sem napsal, abyste se nad tim s @tomasfejfar zamysleli jestli vas nenapadne sitauce ve ktere by to mohl byt problem.

@odinuv Kdyz bude ve state {}/[], tak je to uplne jedno, oboji je validni prazdny json. To by nemelo vadit nikde.

ja vim, ze je oboji validni, jde o to, ze se prazdny objekty konvertuji na pole a nelze rozlisit mezi tim jestli to byl prazdny objekt nebo prazdne pole
python by se na tom treba vysypal

To uz je pak ale na pythonovou komponentu. Jak si psal toto je vstup pouze pro php, takze je to jedno. Na vystupu nikdy prazdny state zapisovat nebudeme, coz bych videl jeko potencialni nekonzistenci.

tomasfejfar · 2018-10-15T09:17:39Z

src/BaseComponent.php

+    {
+        $inputStateFile = $this->getDataDir() . '/in/state.json';
+        if (file_exists($inputStateFile)) {
+            $jsonContents = (string) file_get_contents($inputStateFile);


Já bych, když se nám to tu už objevuje víckrát, extrahoval tuhle funkčnost do samostatné třídy s tím, že tam bude všechno - kontrola, jestli soubor existuje, správné natažení ze souboru, decodování přeš JsonEncoder, atp. Teď jsem zrovna potřeboval natahnout JSON ze souboru a musel jsem to zas napsat celé znova, takže jsem se na to vyprd a udělal jsem jen json_decode(file_get_contents())...

tomasfejfar · 2018-10-15T09:23:02Z

src/BaseComponent.php

+    protected function writeOutputStateToFile(array $state): void
+    {
+        $outDataDir = $this->getDataDir() . '/out';
+        if (!is_dir($outDataDir)) {


V testech by nemusel, teoreticky. Spíš co použít SF Filesystem?

tomasfejfar · 2018-10-15T09:23:23Z

src/BaseComponent.php

+    protected function writeOutputStateToFile(array $state): void
+    {
+        $outDataDir = $this->getDataDir() . '/out';
+        if (!is_dir($outDataDir)) {


Tam by mělo stačit $fs->dump($cesta, $obsah)

tomasfejfar · 2018-10-15T09:25:00Z

src/BaseComponent.php

+        }
+
+        $outputStateFile = $outDataDir . '/state.json';
+        $jsonEncode = new JsonEncoder();


Tohle je to samé - mohlo by se hodit to mít např. pro zápis manifestu tabulek - JsonReader & JsonWriter... a ošetřit situace, kdy nejde encodovat, neexistuje složka s tím souborem, atp.

a ošetřit situace, kdy nejde encodovat, neexistuje složka s tím souborem,

je to app error, nemusi se to osetrovat

@tomasfejfar Na to co tu popisujes budou stacit 2 metody, nebude jednodussi udelat nejaky JsonFileHelper, ktery bude umet oboji zaroven?
@odinuv Sice to vyhodi app error, ale nekde s temi stavy budes chtit pracovat, napr. zrovna tady s tim stavem, kdy state.json neexistuje, tak chces nastavit $state na [].

Jeste k tomu teda jedna vec. Opravdu tady potrebujeme SF/serializer? Serializovani niceho jineho tu ani nepotrebujeme a na json nam bohate staci json_decode/json_encode

jedinej stav se kterym tu chces pracovat je, ten, ze ten soubor neexistuje, vsechno ostatni je app error, takze netreba overengineering

taky si myslim, ze by tu bohate stacil json_encode a json_decode

@Actimel https://phpfashion.com/how-to-encode-and-decode-json-in-php

tak uz mame 7.2

No, tak aspoň ošetření přes json_last_error. Protože pořád platí, že:

json_decode('null') === json_decode('{invalidJson');

A ano, klidne to může být jen jedna třída. Ale mělo by to být víc než jen metoda - aby to bylo otestované a dalo se to použít kdykoli - třeba v testu.

tomasfejfar · 2018-10-16T07:39:46Z

src/BaseComponent.php

+        (new JsonFileHelper())->write(
+            $this->getDataDir() . '/out/state.json',
+            $state,
+            ['json_encode_options' => JSON_PRETTY_PRINT]


nehodilo by se to jako default? Jak často zapisujeme json neformátovaný? Osobně bych radši viděl true/false parametr, protože posílat celý kontext mi přijde potenciálně problematické.

Nekde jsem neformatovany json uz videl - myslim, ze v teste db-ex-common/wr-bigquery. Nicmene zrovna na tech mistech mi prijde lepsi to mit naformatovane, at se v tom clovek dokaze jednoduze zorientovat.

Mozna nemusime posilat cely kontext, ale primo json_encode_options , tim budes schopny ten json dodatecne nastavit. Nenapada me ale pripad, kdy pouzivame neco jine nez JSON_PRETTY_PRINT.

Jestli vas taky nic nenapada tak to muzeme resit true/false parametrem.

spis je mozna otazka - potrebujem tam to nastaveni vubec na neco? jestli jo, tak bych to dal jako default a bool, nic jinyho jsme nikdy nepotrebovali (a kdyby to nekdy nekdo potrevoval, tak se to da vzdycky napsal low-level)

osobne bych to teda vubec nedaval nastavitelny, at je to co nejjednodussi

Pridal jsem tam ten bool, a defaultne to nastavil na true. Kdyz to budes chtit vypsat na jeden radek, musis tam dopsat false.

btw, co kdyby to byly staticky metody? helper zadnej vnitrni stav nema a mit nebude a
JsonFileHelper::write(...) je jednodussi nez (new JsonFileHelper())->write(...)

tomasfejfar · 2018-10-16T07:40:40Z

tests/JsonFileHelperTest.php

+            file_get_contents($filePath)
+        );
+        unlink($filePath);
+    }


chybí test writeToDirectoryThatDoesNotExist

Doplnil jsem.

tomasfejfar · 2018-10-16T11:38:22Z

tests/JsonFileHelperTest.php

        unlink($filePath);
    }
+
+    public function testWriteToNonExistingDirectoryThrowsException(): void


Tohle podle mého je nesprávné chování - právě, že by to mělo šetřit čas a tu složku vygenerovat. Co, @odinuv ?

@odinuv Psal, ze ta slozka out, tam vzdy bude a kdyz ne, tak se to ma probublat do ApplicationErroru.

zalezi za co chcete ten jsonfilehelper mit, jestli ma slouzit jen pro potreby php-component, tak to slozku vytvaret nemusi
jestli ho chcete pouzivat i jinde, tak je to na zvazeni

Není to statefilewriter. Je to generický dumper json filů. Takže s tím stejně tak dobře můžu chtít zapsat třeba /out/tables/mytable/some.manifest

nemohli bysme tady v tomhle PR nechat symfony encoder a ten helper resit nekde zvlast?
protoze kdyz tohle mergnem, tak bude state pouzivat helper, a manifset symfony encoder, anebo to teda zmenit i tam, ale to u zase dela ten PR 3 veci

Ale jo, dejme to tam...

taky nemam php-utils rad a dal bych to radsi nekam jinam

Toho jsem si take vsiml, proto bych to zadelal do slozky, at je to od toho pelmelu odstinene.

Ale lepsi nez se tomu php-utils vyhybat jak cert krizi protoze je v nem chaoz, bychom mu mohli timto dat impuls k oziveni. Nez se na to divat jak to hnije.

oziveni php-utils vyzaduje dohledat kde vsude se ty funkce tam definovany opravdu pouzivaji a potom s tim nejak nalozit, tipuju, ze se to cely pouziva tak ve 3 appkach a kazda z toho pouziva uplne disjunktni mnozinu funkci :)

Pridal jsem vytvareni slozky, pokud neexistuje a predelal metody na staticke.

Muzeme to takto mergnout? To do jakych utils vytahnout ten helper bych resil potom.

odinuv · 2018-10-16T12:31:59Z

src/JsonFileHelper.php

+        }
+
+        $jsonEncoder = new JsonEncoder();
+        file_put_contents(


tady by to melo vyhodit vyjimku pokud je navratova hodnota === false, jinak to nemuselo zapsat

Vis za jakych podminek to vraci false? Nenapada me jak to otestovat.

tohle se blbe testuje :/, mozna by slo neco jako zapis do /dev/full nebo nejaky invalid path nebo php://stdin ?

nebo adresar, co nema write permissions

To jsem zkousel, ale kdyz nemas write permission, tak uz sam file_put_contents vyhodi exception. Zatim to nechavam bez testu.

public function testWriteToDevFull(): void { $filePath = '/dev/full'; $array = ['key']; JsonFileHelper::write($filePath, $array); }

protože se to nastavuje i v tom bootstrapu testů.

Jasne, to je ono. Jinak je to warning. Nicmene i tak nevim, jak otestovat, kdy file_put_content vraci false 😕

v php-component nijak, tady to vyhazuje vyjimku, takze tady staci otestovat, ze to vyhazuje vyjimku, az se to bude davat do utils, tak tam je to potreba poresit - bud registraci handleru, nebo @ a otestovani false

Ok, tak tu jsem pridal ten test.

odinuv

ja bych bylo pro to mergnout, ale pokud ano tak soucasne s vytvorenim issue na pouziti JsonFileHelper v manifestech, at je vsude stejny encoder

odinuv

beru zpet

odinuv

boha jeho, jinej PR, sorry!

Actimel · 2018-10-17T17:42:20Z

Zalozil jsem #49

Add loading input state file

3226de9

Actimel requested a review from tomasfejfar October 14, 2018 08:31

Actimel self-assigned this Oct 14, 2018

Add write state file method

b3e6c25

odinuv reviewed Oct 15, 2018

View reviewed changes

tomasfejfar reviewed Oct 15, 2018

View reviewed changes

Actimel added 3 commits October 15, 2018 20:27

Use more strict assertion

6a353fa

Remove folder check

f9d0455

Create JsonFileHelper class for read/write operations

13de6c0

Actimel force-pushed the david-get-state branch 2 times, most recently from 105ef2d to 1164135 Compare October 15, 2018 19:29

Fix code style

d47bbd8

Actimel force-pushed the david-get-state branch from 1164135 to d47bbd8 Compare October 15, 2018 19:31

tomasfejfar requested changes Oct 16, 2018

View reviewed changes

Add missing test

f31734a

Actimel force-pushed the david-get-state branch from 75997d7 to 0ed97bd Compare October 16, 2018 10:48

Actimel requested a review from tomasfejfar October 16, 2018 10:49

Do not pass whole context, use true/false for pretty printed json

a945165

Actimel force-pushed the david-get-state branch from 0ed97bd to a945165 Compare October 16, 2018 10:53

tomasfejfar reviewed Oct 16, 2018

View reviewed changes

Actimel added 2 commits October 16, 2018 14:22

Create directory if does not exist

d61c909

Use static methods

5f22108

tomasfejfar approved these changes Oct 16, 2018

View reviewed changes

odinuv reviewed Oct 16, 2018

View reviewed changes

Actimel added 3 commits October 16, 2018 14:50

Throws ErrorException when there was nothing written

eef6976

Add test

99f513c

Fix test

31c4abb

odinuv approved these changes Oct 17, 2018

View reviewed changes

odinuv requested changes Oct 17, 2018

View reviewed changes

odinuv approved these changes Oct 17, 2018

View reviewed changes

Actimel mentioned this pull request Oct 17, 2018

Use JsonFileHelper for creating manifests #49

Closed

Actimel merged commit 5c7c604 into master Oct 17, 2018

Actimel deleted the david-get-state branch October 17, 2018 17:43

odinuv mentioned this pull request Dec 28, 2018

Fix JsonHelper implementation and tests #54

Merged

Add loading input state file #48

Add loading input state file #48

Uh oh!

Conversation

Actimel commented Oct 14, 2018

Uh oh!

Actimel commented Oct 14, 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!

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!

odinuv Oct 16, 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!

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!

odinuv Oct 16, 2018 •

edited

Loading

tomasfejfar Oct 16, 2018 •

edited

Loading