search

keboola / php-component

General library for PHP applications running in Keboola Connection environment
MIT License
0 stars 1 forks source link

Does writeTableManifest ignore datadir? #14

Closed ujovlado closed 6 years ago

ujovlado commented 6 years ago 
...
$mm = new ManifestManager($outputPath);
$mm->writeTableManifest($webalizedExportName . '.csv', '', [], [], $exportOptions['incremental']);
...

And it looks like manifest file is written to path where script is executed. Shouldn't be written to $outputPath . '/out/tables'?

ujovlado commented 6 years ago

I would expect manually typed data dir here:

https://github.com/keboola/php-component/blob/23eb9bf5cb76a90701e3e8f64d000c1693e03327/tests/Manifest/ManifestManagerTest.php#L90

because we expect that manifest manager writes to known path (same as passed to constuctor).

ujovlado commented 6 years ago

And since it's not as I'm writing, tests still passed: https://travis-ci.org/keboola/php-component/builds/353194284

tomasfejfar commented 6 years ago

Thx for failing test. You are right.

tomasfejfar commented 6 years ago

Generally this was caused by the fact that I modeled everything along the python application. That behaved this way - requiring you to pass full path when writing manifest. After few rounds of reviews it didn't resemble it that much, but some parts stayed the same.

ujovlado commented 6 years ago

No problem :) I was only confused a bit, because provided dataDir wasn't used.

tomasfejfar commented 6 years ago

What do you think about the BC compatible solution in previous commit? Or should I just grab the opportunity to polish the API with new major version?

ujovlado commented 6 years ago

It's up to you, but I think the latter is better. Imho, It should not behave differently when I pass absolute path. Also, it can cause confusion, because these two examples will do the same thing:

$mm = new Manager('/data-dir');
$mm->writeMf('/data-dir/out/tables/xyz.manifest');
$mm = new Manager('/data-dir');
$mm->writeMf('xyz.manifest');

And first is a little wtf for me, since I don't know why Manager needs data-dir if it won't use it.