Skip to content
This repository has been archived by the owner on Apr 28, 2021. It is now read-only.

init #1

Merged
merged 20 commits into from
Aug 17, 2018
Merged

init #1

merged 20 commits into from
Aug 17, 2018

Conversation

odinuv
Copy link
Member

@odinuv odinuv commented Jul 31, 2018

No description provided.

@odinuv odinuv requested review from tomasfejfar and Actimel August 1, 2018 11:44
Copy link

@tomasfejfar tomasfejfar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vypadá to dobře. Jen pls fixni typo + require dev. Zbytek je spíš preferenční nebo dotazy okolo.

Kam padají ty message, když bych se chtěl podívat ve slacku?

@@ -1,2 +1,3 @@
/vendor
/.idea
/data

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dává smysl to rovnou dát do generátoru? Najloš říkal že to skoro nikdy není a mívá ten datadir někde vedle a má jich třeba víc, ale jak vidím, tak nejsem jediný, kdo to takhle používá :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taky to tak používám

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

README.md Outdated
- `#token` - Workspace token obtained when the application is installed
- `channel` - Channel to send the messages to, e.g. `#my-channel` or `@some-person`

At least one table has to be provided on input mapping and it must have on or two columns. If present, the second

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo on → one

composer.json Outdated
@@ -1,6 +1,9 @@
{
"require": {
"PHP": "^7.1",
"guzzlehttp/guzzle": "^6.3",
"keboola/csv": "^2.0",
"keboola/datadir-tests": "^2.1",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do require-dev

$writer = new Writer($config->getToken(), $this->getLogger());
$tables = $config->getInputTables();
if (count($tables) < 1) {
throw new UserException("At least one table must be supplied on input. The table must contain one column");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one or two columns?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spise ta druha veta by tam vubec nemusela byt. Chyba to vyhodi v pripade, ze neni tabulka, takze prvni cast je dostatecna.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

je tam schvalne, aby to uzivateli reklo presneji co se po nem chce

if (count($header) > 2) {
throw new UserException(
sprintf(
"The table %s contains %s columns. Every table must contain at most 2 columns.",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taky k tomu jen to, že dělám vždycky sprintf('Item "%s" something', $x) protože pak je vidět, když se objeví bug a to $table['source'] by bylo prázdné (Item "" something). Protože tady zrovna to dává smysl i bez obou těch replacementů. Table contains columns. Přijde mi to lepší z hlediska debugování potom...

src/Writer.php Outdated

public function writeMessage(string $channel, string $message, ?string $attachments) : void
{
$client = new Client(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nějak podivně zarovnané. Čekal bych:

new Client([
    'stuff'
]);

src/Writer.php Outdated
}
}

private function getHandlerStack() : HandlerStack

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Už několikáté místo, kde vidím exponenciální retry - nestálo by za to to vyextrahovat udělat z toho extra package?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phputils? no zatim to vypada, ze je vzdycky trosku jiny

__DIR__ . '/invalid-table/expected/data/out'
);
$tempDatadir = $this->getTempDatadir($specification);
$data = [

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dávalo by ti smysl mít nějaký lepší způsob, jak zapsat ty ENVy do configu? Třeba $this->setConfigValue(['parameters', 'channgel'], getenv('SLACK_TEST_CHANNEL'). Takže pak bys tam měl jen dva takovéhle řádky a pak parent::testDatadir(); nebo že bys měl třeba metodu $this->prepareConfig(), kterou bys přetížil. Protože nic moc jiného se tam neděje.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ani ne, stejne tady potrebuju vytvorit sekci storage, navic me to takhle prijde prehlednejsi - vidim vpodstate 1:1 ten config, ktery do toho vleze

}
},
"scripts": {
"tests-phpunit": "phpunit",
"tests-datadir": "datadir-tests",
"tests-datadir": "phpunit tests/functional",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spoustet i phpstan

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ten se pouští na ř. 4 v rámci build scriptu

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mas pravdu!

$writer = new Writer($config->getToken(), $this->getLogger());
$tables = $config->getInputTables();
if (count($tables) < 1) {
throw new UserException("At least one table must be supplied on input. The table must contain one column");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spise ta druha veta by tam vubec nemusela byt. Chyba to vyhodi v pripade, ze neni tabulka, takze prvni cast je dostatecna.

}
foreach ($tables as $table) {
$csvFile = $this->getDataDir() . '/in/tables/' . $table['destination'];
$csv = new CsvReader(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rezii okolo naparsovani a validace csv souboru presunout jinam. Popripade rovnou zavolat Writer a tomu predat cestu k souboru.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do writeru bych to necpal. Ten by vůbec nemusel tušit, že existuje nějaké CSV někde. Zabalit by se mi to taky líbilo. Ale míváme to takhle, že se udělá "obecná implementace" (tj. ta Writer class) a k ní se udělá glue kód přímo do té component class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chapu ze do Writeru to uplne neni vhodne, proto by mohla byt pomocna metoda, ktere to csv naparsuje a zvaliduje, ted je to roztahane a vylepene pres polovinu run() metody. To, ze to tak mivame, neznamena, ze je to spravne. Kdyz se to bude podobne normalizovat u vicero projektu, muze z toho sama vypadnout dalsi vychytavka, protoze casto resime to stejne.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no moje uvaha je vicemene jak psal Tomas - writer by se nemel starat o to, kde a jak se ty data vzaly, cili jeho interfacem je metoda writeMessage($channel, $message, $attachments), trida Component je neco jako controller, "ktera to zaridi". Kdybych to mel napsat super ciste, tak udelam metody checkInputs, getMessages a v run bude

checkTables()
$m = getMessages()
foreach ($m as $msg) 
   $writer->writeMessage(...)

jenze, checkTables a getMessages znamena, ze vsechny CSV otevru a parsuju 2x, a getMessages znamena, ze se vsechny messages nactu do pameti, coz rozhodne nechci. Takze bych to musel obalit zase nejakou dalsi classou a udelat z toho StreamInterface, nebo aspon funkci s yieldem a to me vsechno prijde hroznej overkill pro zprehledneni 15 radku kodu.

Lepsi varianta by byla Writer prejmenovat na SlackClient, a udelat tridu Writer, ktere se preda soubor, ona ho zvaliduje a zpracuje - no timpadem by v Component::run zustalo akorat

forach ($config->getInputTables() as $table
   $writer->processTable($table)

to by me asi prislo fajn, ale i tak mi to prijde trosku jako JavaLikeOverKill vzhledem k tomu, ze ten writer mozna hned zase zahodime nebo prepiseme :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asi si do toho zabredl o trochu vice, nez jsem myslel. Mel jsem na mysli, ze by se dalo vytvareni CsvReader, vysekat nekam bokem, at muzes udelat jen napr. $csv = $this->obtainCsvReader($filePath);. Parsovat 2x nic nemusis a ani zpravy nebudes nacitat do pameti.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jak jeste pises tu druhou variantu, tak tak by to klidne slo take, a nevidim problem v tom, ze se jednou Writer prepise, kdyz u toho noveho dodrzis interface pro processTable(). Ale to uz je mozna az zbytecne.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Actimel tak co ted? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Za me to vypada lip 👍

src/Config.php Outdated

use Keboola\Component\Config\BaseConfig;

class Config extends BaseConfig
{
// @todo implement your custom getters
public function getFoo() : string
public function getToken() : string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mezera pred :

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Divím se, že mi to netrhlo oči

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D jak pises, kdyz je clovek zvykly vypada to divne

Jinak vzhledem k temto chybam bych byl pro, aby se phpcs klidne nechal v defaultni konfiguraci (rozumej smaz .../phpcs.xml) a tim se bude by default pouzivat pravidla PSR2.

Nebo se vydat na dlouhou cestu, aby se vytvorilo striktnejsi https://github.com/keboola/phpcs-standard

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ten phpcs standard je PSR2 a něco navíc. PSR2 byl původně a mě právě dost štvalo, že spousta věcí byla random, jak to kdo zrovna psal. PSR2 tohle zatím totiž furt neřeší a je prakticky zastydlý v PHP5 stylu.

Byl bych pro to přidat do CS - je na to ReturnTypeHintSpacingSniff ve Slevomatím CS, který už stejně používáme. Uděláš PR?

Ještě je k tomu rozběhlý squizlabs/PHP_CodeSniffer#1219, ale byl bych pro hotový ze Slevomatu.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zrovna nedavno jsem to pridaval u sebe na projektu jen ciste v defaultu a myslel jsem, ze to v sobe ma. Co jsem si tak procital CS, tak vse mi prislo stejne jako uz jsem pouzival, tak jsem si rekl super. To ze tam chybi kontrola tech return typu jsem si vsiml az ted, kdyz si na to upozornil 👍 (do ted se mi je nepodarilo napsat s mezerou).

Udelam PR a pridam tam ten slevomati, kdo vi, kdy to tam mergnou.

src/Config.php Outdated
return $this->getValue(['parameters', '#token']);
}

public function getChannel() : string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mezera pred :

src/Writer.php Outdated

public function writeMessage(string $channel, string $message, ?string $attachments) : void
{
$client = new Client(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Klienta staci vyrvorit jednou a pak znovupouzivat.

src/Writer.php Outdated
$this->logger = $logger;
}

public function writeMessage(string $channel, string $message, ?string $attachments) : void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mezera pred :

src/Writer.php Outdated
} else {
$attachments = null;
}
$request = new Request(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vyclenit vraceni requestu do separatni metody.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nebo pouzit primo $client->post()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ja ten post nemam rad, protoze se mu to posila jako asociativni pole, uz jsem se nekolikrat zadrbal na tom, ze jsem tam mel neco jako headars a badal jsem nad tim proc se to neautorizuje

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Actimel no nakonec jsem to neudelal tak ani tak, JSONserializabe tam neni potreba a headery jsem dal do constructoru tak je to cistsi

src/Writer.php Outdated
'Authorization' => 'Bearer ' . $this->token,
'Content-type' => 'application/json; charset=utf-8',
],
\GuzzleHttp\json_encode([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Telo zpravy vyclenit bokem do tridy implementujici \JsonSerializable a posilat to pomoci ->post() viz vise. Guzzle si to pote sam pretransformuje, cele by to mohlo vypadat nejak takto:

$client->post($uri, [
    'headers' => $headers,
    'json' => $messageRequestBody
]);

src/Writer.php Outdated
{
$this->token = $token;
$this->logger = $logger;
$this->client = new Client([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Konstruktorem predavat, ne vytvaret

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clienta nevytvaret v konstruktoru, ale do konstruktoru si ho poslat parametrem

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no to nechci, co je komu potom, ze to tam Writer posila pres Guzzle klienta, mozna to v dalsi verzi prepisem, ze to bude pouzivat nejakou php slack knihovnu

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nikomu po tom nic byt nemusi, ale bude lepsi, kdyz se ta zavislost, at je to ted Guzzle nebo v budoucnu slack knihovna, bude dostavat uz vytvorena a nevytvaret se v konstruktoru.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No ja to to chapu ted tak, ze Writer = SlackClient a nechci to overengineerovat, az ten SlackClient bude podporovat vic api method (coz se nejspis jen tak nestane :) tak pak mi dava smysl mit SlackClient (s SlackClientFactory) a Writer, kterymu se preda klient a data. A klienta pak vytvaret mimo writer, protoze se pouzije i na neco jinyho (treba na vypis kanalu)

porad plati, ze je to zatim alfa komponenta, kterou mame jen pro sebe a mozna ji celou zahodime

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pro zacatek bych neresil rovnou SlackClient, ale stacilo by jen udelat GuzzleFactory. Prijde mi to takto cistsi a hlavne to +, ze muzes Writer otestovat. Coz se bude hodit vzhledem k tomu, ze pises, ze je to alfa a muze se to potom hodne jeste upravit. Na to ze se to zahodi bych spolehat nechtel.
Celkove jako overingineering mi to tolik neprijde, kdyz to prinese vyhodu v testovani. Navic to neni uz velka zmena, jen se prida jedna factory a upravi parametry konstruktoru.
Je pravda, ze k tomuto jsem kriticky jak kdyby to byla nejaka velka infrastruktura, ale prijde mi to momentalne jako drobna zmena a aspon z casti se vyvarujeme technickemu dluhu.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Actimel já jsem na to měl úplně stejný náhled jako ty. Nicméně jsem se nechal přesvědčit, že u komponent se často jedná o tak malou funkcionalitu, že nevadí a mnohdy se vyplatí to nedělat optimálně.

Co považuju za problém je, že tím, že neděláme optimálně malé jednoduché věci, tak pak děláme méně optimálně velké složité věci. Takže je na zvážení, jestli nezkusit mít vyšší nároky na naše komponenty abysme si víc trénovali a pak to dokázali lépe přenášet do těch větších věcí. Ale je potřeba brát v úvahu, že když to budeš psát takhle pěkně, tak si (speciálně tím, že nemáme DIC s autowiringem) přiděláváme nezanedbatelné množství ruční práce.

Na to navazuje otázka, proč teda nemáme v komponentách DIC s autowiringem. A odpověď je, že proto, že jsme to řešili, když jsem navrhoval keboola/php-component a dohodli jsme se ~ že to chceme držet jednodušší, aby to bylo na první pohled pochopitelné. Aby ses podíval třeba na ex-http a mohl si podle toho udělat svůj extractor. Aktuálně v těch komponentách je minimum magie - největší je asi magické načtení a zvalidování konfiguráku. Oproti tomu obyčejná prázdná Symfony aplikace je několik stovek souborů i když zatím vůbec nic neumí. a pokud Symfony neznáš, tak není snadné to celé pochopit, jak to funguje. K tomu musíš vědět jak tam funguje generování DIC, že jsou tam nějaké extra konfiguráky s parametry, atp. Samozřejmě je taky možnost tam dát třeba pimple nebo tak něco. Ale ta je vidět třeba v wr-google-drive, že taky není ideální - je tam spousta zbytečného psání.

Copy link
Member Author

@odinuv odinuv Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no ja si myslim, ze bysme meli rozlisovat mezi dulezityma a nedulezityma vecma, pokud budem kazdou "blbost" implementovat pomoci 42 rozhrani a factory, tak skoncime jako SAP. Cela tahle komponenta jsou 3 radky v bashi (ne ze bych to chtel psat v bashi) a myslim, ze si proste lepsi zachazeni nezaslouzi.

takze pokud nejste vyslovene proti, tak bych tohle mergnul a releasnul jako 0.1.0 a povidani o tom jestli to chceme takhle delat dal nebo ne bych nechal na offsite https://www.wunderlist.com/#/tasks/4110801154

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jo, určitě bych to tady nekomplikoval.

@Actimel
Copy link
Contributor

Actimel commented Aug 10, 2018

Prihodil jsem commit, jak si myslim, ze by to mohlo byt ok. I kdyz je to male, ta factory tu dava hodne smysl. Nehlede na to, ze se tu o tom tyden dohadujeme, ale na implementaci to zabere 5 minut i s cigaretou.

Kdyz jsem to tak oddelil, pridal jsem k tomu i Mockery a napsal testy na Writer.

@Actimel
Copy link
Contributor

Actimel commented Aug 10, 2018

Akorat jsem tim teda rozbil build, na to se jeste podivam.

composer.json Outdated
@@ -34,7 +34,7 @@
"@tests-datadir"
],

"phpstan": "phpstan analyse ./src ./tests --level=max --no-progress -c phpstan.neon",
"phpstan": "phpstan analyse ./src ./tests/functional --level=max --no-progress -c phpstan.neon",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proč?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pruser je v unit testech, rve to na tom, ze to ocekava napr. class Client, ale predavas tomu MockInterface - je pravda, ze bychom mohli jen ignorovat tyto druhy chyb

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Přidáš DocBlock: /** @var MockInterface|Client $mock */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resp. v ideálním případě MockInterface&Client, ale to zatím umí jen PHPStan. V PhpStormu je to zatím error, tak bych to nepoužíval.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ten DocBlok tam je, ale nepomohlo to.
4beaf02#diff-a9380e00f158c153551dbfcb9d8b87b7R20

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// @TODO implement
/** @var Config $config */
$config = $this->getConfig();
$client = (new ClientFactory($config->getToken()))->create();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dává mi smysl, když takhle, tak ClientFactory()->createForToken($token) a pak můžeš ClientFactory instancovat už v konstruktoru.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevidim duvod to instancovat uz v konstruktoru, ale je pravda, ze ten token se muze predavat az tim create() - nazev bych zachoval, zadny jiny create se tam vyhledove predpokladam pridavat nebude, tak je to dostacujici.


class Component extends BaseComponent
{
private function getTableCSV(string $tableSource, string $tableDestination): CsvReader

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proč to má dva parametry? To vytahává jen z jedné tabulky a sype do Slacku, ne?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zkratky se píší camelcased (generateHtml(), parseCsv(), getHtmlButtonMarkup())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Takže getTableCsv()

src/Writer.php Outdated
@@ -73,7 +73,7 @@ private function handleResponse(ResponseInterface $response, string $message) :
);
}
try {
$responseData = \GuzzleHttp\json_decode($response->getBody(), true);
$responseData = \GuzzleHttp\json_decode((string) $response->getBody(), true);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getBody()->getContents()?

return $handlerStack;
}

private static function createDefaultDecider(int $maxRetries = 3) : callable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


class Component extends BaseComponent
{
private function getTableCsv(string $tableSource, string $tableDestination): CsvReader

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tady jsi nedořešil můj dotaz proč to má source a destination, když tam destination je slack... a navíc z té destination se to tahá?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To je otazka na @odinuv jaky k tomu je duvod, ja jsem to jen prejmenoval, cimz jsem nesikovne schoval tvuj koment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jo to určitě je na Odina

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomasfejfar - no tak se podivej do te metody :) pouziva se to oboji - jedno pro nacteni a jedno pro vypis zpravy. Je to zdroj a cil tabulky, ne zpravy

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jo, to je input mapping - bucket v KBC -> CSV soubor v dockeru! Jasně, sorry.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

"first_col"
"some message","[{""text"":""some text"",""actions"":[{""type"":""button"",""name"":""response"",""value"":""yes"",""text"":""yes"",""style"":""primary""}]}]"
```


## Development

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ještě bych poprosil doplnit, kam se v testech posílají ty zprávy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO nikam. Musis si nastavit env variables, v travisu nevidim, ze by neco meli zadane. Ja to testuju tak, ze jsem si zalozil svuj slack a kdyz to spoustim, tak tomu ty 2 promenne nastavim.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Datadir testy to někam sypou, ale bylo by fajn, abysme věděli kam - přesně proto, abysme nemuseli zakládat pokaždé vlastní slack :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ja vidim v travisu kanal wr-slack:
image

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

$message = 'Hello world!';

$this->writer->writeMessage($channel, $message, null);
$this->assertTrue(true);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Actimel k cemu je tam tohle?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testuje to writeMessage(): void, kde nemas co jineho assertnout. Kdyz by to failnulo, tak na exceptionu v te metode. Assert na true tam je jen kvuli tomu, ze je jinak test oznaceny jako risky.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kde je oznacenej jako risky? u me si nic nestezuje

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kdyz si zakomentujest tu aserci a pustis phpunit, tak ti to napise, ze je ten test risky, protoze tam neni zadny assert

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blby je, ze $this->doesNotPerformAssertion() nefunguje. Musis pouzit anotace, coz mi prijde prasacky. Nicmene Odin to vyresil hezky 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pokud se to nepodedi z toho mockerytestcase, tak nefunguje spravne ten check na once, protoze mockery nezjisti, ze ten test skoncil - cili kdyby se ta metoda testovana v once vubec nezavolala, tak by ten test prosel - nezavola se mockery::close

use Keboola\SlackWriter\Writer;
use Mockery\Adapter\Phpunit\MockeryTestCase;
use Mockery\MockInterface;
use PHPUnit\Framework\TestCase;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toto je tim padem navic

@odinuv
Copy link
Member Author

odinuv commented Aug 16, 2018

@tomasfejfar any more changes requested? :)

Copy link

@tomasfejfar tomasfejfar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kromě toho zapomenutého ENVu LGTM

public function setup(): void
{
parent::setUp();
if (empty(getenv('SLACK_TEST_TOKEN')) || empty('SLACK_TEST_CHANNEL')) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty(env('SLACK_TEST_CHANNEL'))
      ^^^

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Už to čtu nejmíň potřetí a všiml jsem si až teď...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dobry postreh, tohle mi take uslo 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha :)

@odinuv odinuv merged commit 539a868 into master Aug 17, 2018
@odinuv odinuv deleted the odin-init branch August 17, 2018 15:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants