6) Update ci and cs #71

pivnicek · 2018-09-27T15:56:29Z

No description provided.

tomasfejfar · 2018-09-27T16:51:00Z

driver/downloadDriver.php

@@ -7,23 +10,29 @@

 require_once $basedir . '/vendor/autoload.php';

-$client =  new \Aws\S3\S3Client([
+$client =  new \Aws\S3\S3Client(


this is broken :) Either should stay on the same line or contents of array should be indented

There are a lot of useless diffs just putting arrays on newlines. I think those should be removed. They are not part of CS and add no value whatsoever.

I'm quite sure that those were created by phpbcf, but I can revert it.

Huh! That would be a bug in phpcbf (or the sniff) then!

the sniff is fine with it, so I guess it must have been me:). anyhow, it's reverted.

tomasfejfar · 2018-09-27T16:51:48Z

run.php

+            true
+        );
+    } else {
+        throw new UserException('Invalid configuration file type');


Invalid? More like missing, don't you think?

tomasfejfar · 2018-09-27T16:54:29Z

src/Keboola/DbExtractor/Extractor/Snowflake.php

-            });
+            $objectColumns = array_filter(
+                $columnInfo,
+                function ($column) {


closures can be typehinted too ;)

src/Keboola/DbExtractor/Extractor/Snowflake.php

tomasfejfar · 2018-09-27T16:57:10Z

src/Keboola/DbExtractor/Extractor/Snowflake.php

@@ -265,12 +294,12 @@ private function generateCopyCommand($stageTmpPath, $query)
    }

    /**
-     * @param $copyCommand
-     * @param int $maxTries
+     * @param  $copyCommand


does not seem right...

tomasfejfar · 2018-09-27T17:02:41Z

src/Keboola/DbExtractor/Extractor/Snowflake.php

+                ', ',
+                array_map(
+                    function ($tableName) {
+                        return "'" . $tableName . "'";


why not quoteIdentifier?

it's not an identifier in this case, but a value

I see, but anyway... isn't there a proper quoting function? This is not quoting, this is "wrapping in quotes".

I don't think there is a "proper" one, but there is a private method in this class that should be used here, https://github.com/keboola/db-extractor-snowflake/blob/master/src/Keboola/DbExtractor/Extractor/Snowflake.php#L538

If there is nothing better...

The addslashes() is sometimes incorrectly used to try to prevent SQL Injection. Instead, database-specific escaping functions and/or prepared statements should be used.
-- http://php.net/manual/en/function.addslashes.php

yes, that's a completely valid point, I'll change it to statement. thanks.

I'll change it when I fix this: #72

tomasfejfar · 2018-09-27T17:03:50Z

src/Keboola/DbExtractor/Extractor/Snowflake.php

-     * @return \SplFileInfo
-     */
-    private function crateSnowSqlConfig($dbParams)
+    private function crateSnowSqlConfig(array $dbParams): \SplFileInfo


create or is it a crate?

still not fixed

tomasfejfar · 2018-09-27T17:05:16Z

tests/AbstractSnowflakeTest.php

@@ -186,10 +191,10 @@ private function generateCreateCommand($tableName, CsvFile $csv, $fileInfo)
     * Create table from csv file with text columns
     *
     * @param CsvFile $file
-     * @param string $tableName - optional name override (default uses filename)
-     * @param string $schemaName - optional schema in which to create the table
+     * @param string  $tableName  - optional name override (default uses filename)


something fishy :)

Also missing |null

still not resolved

tomasfejfar · 2018-09-27T17:06:13Z

tests/AbstractSnowflakeTest.php

@@ -241,10 +253,10 @@ protected function createTextTable(CsvFile $file, $tableName = null, $schemaName
    /**
     * Count records in CSV (with headers)
     *
-     * @param CsvFile $file
+     * @param  CsvFile $file


It seems you need to disable attribute alignment in you phpstorm code style settings... let me know if you couldn't find it.

tomasfejfar · 2018-09-27T17:08:00Z

composer.json

        "keboola/storage-api-client": "^4.14",
-        "keboola/php-csv-db-import": "~2.3.0"
+        "phpstan/phpstan": "^0.8"


0.10.3 is latest afaik

as you are adding a new dependency you should set it to latest available IMHO... even though it will install correctly latest version even with this version constraint

.travis.yml

docker-compose.yml

tomasfejfar · 2018-11-07T09:56:38Z

src/Keboola/DbExtractor/Extractor/Snowflake.php


-        return $outputTable;
+        return [


this is logic change hidden inside "ci&cs" PR. Please don't do this.

I understand that it's caused be upstream change in db-ex-common, but this could have been separated in it's own PR, not in +1000/-600 beast :)

yes, this should be in #69, I'll cherry-pick it if possible.

tomasfejfar · 2018-11-07T10:14:18Z

driver/downloadDriver.php

@@ -7,23 +10,29 @@

 require_once $basedir . '/vendor/autoload.php';

-$client =  new \Aws\S3\S3Client([
+$client =  new \Aws\S3\S3Client(


There are a lot of useless diffs just putting arrays on newlines. I think those should be removed. They are not part of CS and add no value whatsoever.

tomasfejfar · 2018-11-07T10:16:16Z

src/Keboola/DbExtractor/Extractor/Snowflake.php

@@ -226,26 +241,33 @@ private function exportAndDownload(array $table): int

        file_put_contents(
            $outputDataDir . '.manifest',
-            Yaml::dump($this->createTableManifest($table, array_map(


functional change inside CI&CS PR :(

Should have been part of the PR that introduced a change that requires this or it's own separate PR.

yes, it should be part of #67

So what's the plan for this? I see in git graph that you did not cherry pick it. There is a simple way to do this with interactive checkout. I can show you if you want. I wanted to try it in CLI for some time anyway (although I've been using it for a long time in GitExtensions)

tomasfejfar · 2018-11-07T10:21:39Z

src/Keboola/DbExtractor/Extractor/Snowflake.php

+                ', ',
+                array_map(
+                    function ($tableName) {
+                        return "'" . $tableName . "'";


I see, but anyway... isn't there a proper quoting function? This is not quoting, this is "wrapping in quotes".

tomasfejfar · 2018-11-07T10:22:57Z

src/Keboola/DbExtractor/Extractor/Snowflake.php

-     * @return \SplFileInfo
-     */
-    private function crateSnowSqlConfig($dbParams)
+    private function crateSnowSqlConfig(array $dbParams): \SplFileInfo


still not fixed

tomasfejfar · 2018-11-07T10:29:44Z

tests/Keboola/DbExtractor/AbstractSnowflakeTest.php

+
+declare(strict_types=1);
+
+namespace Keboola\DbExtractor\Tests;


Tests do not need to be moved to subdirectory. It would be enough to set:

"autoload-dev": { "psr-4": { "Keboola\\DbExtractor\\Tests\\": "tests/" } },

Also, ideally the namespace should also be different (e.g. Keboola\DbExtractor\SnowflakeExtractor), but I think that it's hardcoded in the common extractor... so that's fine. The note about test paths still stands though.

tomasfejfar · 2018-11-07T10:32:07Z

tests/Keboola/DbExtractor/AbstractSnowflakeTest.php

-        $fileSystem->remove(__DIR__ . '/data/runAction/out');
-        $fileSystem->remove(__DIR__ . '/data/connectionAction/config.yml');
-        $fileSystem->remove(__DIR__ . '/tests/data/connectionAction/out');
+        $fileSystem->remove($this->dataDir . '/out');


This should be part of #67

tomasfejfar · 2018-11-07T10:33:15Z

tests/AbstractSnowflakeTest.php

@@ -186,10 +191,10 @@ private function generateCreateCommand($tableName, CsvFile $csv, $fileInfo)
     * Create table from csv file with text columns
     *
     * @param CsvFile $file
-     * @param string $tableName - optional name override (default uses filename)
-     * @param string $schemaName - optional schema in which to create the table
+     * @param string  $tableName  - optional name override (default uses filename)


still not resolved

pivnicek · 2018-11-07T11:17:55Z

Thanks @tomasfejfar ! I'll get right on it.

pivnicek · 2018-11-07T17:06:23Z

ok @tomasfejfar , apart from the cherry-picking this should be good now I think?

pivnicek · 2018-11-08T10:17:24Z

src/Keboola/DbExtractor/Extractor/Snowflake.php

@@ -480,6 +478,13 @@ function ($tableName): string {
        return array_values($tableDefs);
    }

+    private function shouldTableBeSkipped(array $table): bool


tomasfejfar

I fixed the remainder of useless diffs. Only thing missing is cherrypicking the changes into appropriate branches and then rebase that will squash all my fixup commits.

src/Keboola/DbExtractor/Extractor/Snowflake.php

tomasfejfar · 2018-11-08T08:55:58Z

src/Keboola/DbExtractor/Extractor/Snowflake.php

@@ -226,26 +241,33 @@ private function exportAndDownload(array $table): int

        file_put_contents(
            $outputDataDir . '.manifest',
-            Yaml::dump($this->createTableManifest($table, array_map(


So what's the plan for this? I see in git graph that you did not cherry pick it. There is a simple way to do this with interactive checkout. I can show you if you want. I wanted to try it in CLI for some time anyway (although I've been using it for a long time in GitExtensions)

tomasfejfar · 2018-11-08T09:43:49Z

src/Keboola/DbExtractor/Extractor/Snowflake.php

@@ -393,7 +411,9 @@ public function getTables(?array $tables = null): array
        $tableNameArray = [];
        $tableDefs = [];
        foreach ($arr as $table) {
-            if (($this->schema && $table['schema_name'] !== $this->schema) || $table['schema_name'] === 'INFORMATION_SCHEMA') {
+            if (($this->schema && $table['schema_name'] !== $this->schema)


That sucks :/ I fixed it with a method.

tomasfejfar · 2018-11-08T10:19:30Z

tests/SnowflakeEntrypointTest.php

        $process->setTimeout(300);
        $process->run();

-        $this->assertEquals(0, $process->getExitCode(), sprintf('error output: ', $process->getErrorOutput()));
+        var_export($process->getOutput());


should there really be var_export here? Why?

Why var_export and not var_dump or echo?

Ah, sorry, I should remove that. It was a part of my debugging my local issue.

tomasfejfar · 2018-11-08T10:23:49Z

tests/AbstractSnowflakeTest.php

-                                            "obj" OBJECT,
-                                            "arr" ARRAY
-                                       )');
+        $this->connection->query(


this one makes sense to be reformated.

tomasfejfar · 2018-11-08T10:25:46Z

tests/AbstractSnowflakeTest.php

@@ -215,12 +223,11 @@ protected function createTextTable(CsvFile $file, $tableName = null, $schemaName
                    $q = '"';
                    return ($q . str_replace("$q", "$q$q", $column) . $q) . ' VARCHAR(200) NOT NULL';
                }, $file->getHeader())
-            ),
-            $tableName


Notice how the fixed diff revealed a change in the code!

pivnicek added 21 commits September 25, 2018 16:16

fix the build

e3a4bf9

update to db-extractor-common 5.3.2

bd691dc

tests for json config support

66ac455

ignore temp json configurations

ae7ed77

Support json configurations

30bcc55

fix getTables run test

4d98fee

use exception handling as in php-component

4babb76

update to common 9.1.*

c064ff2

replace bootstrap with autoload

704d0d2

update application constructor

520efab

use correct datadir and app construct

c41c038

update method signatures

8249b18

update to expected sigs and outputs

a155f1f

update tests to expected sigs and outputs

f87fefd

add stan and cs

5e0e5db

use composer ci as command

dc47936

add keboola cs

5b98c9c

phpcbf auto corrections

2580578

phpcbf auto fixes part II

05e50a2

turn off formatting for config definition

9918927

fix type declarations

88ad336

pivnicek changed the title ~~Update ci and cs~~ 6) Update ci and cs Sep 27, 2018

tomasfejfar requested changes Sep 27, 2018

View reviewed changes

pivnicek added 7 commits October 1, 2018 12:40

cs updates

991ca7f

stan fixes

4a0c1b2

stan fixes

bd13363

ini-set param type fix

ec626bf

use composer ci from travis

0ad1b43

rollback phpunit and phpstan"

7d50d1d

remove deprecated constants

2a6fa1f

pivnicek mentioned this pull request Nov 7, 2018

Failed to get tables: 'Undefined index: INFORMATION_SCHEMA.COLUMNS #72

Closed

tomasfejfar requested changes Nov 7, 2018

View reviewed changes

tomasfejfar mentioned this pull request Nov 7, 2018

Use new style of driver download from connection #73

Closed

pivnicek added 8 commits November 7, 2018 15:26

remove unused uses

973a601

debug tests

ea85899

fix doc spacing

3a60372

fix typo in method name

cd858c2

use quote method

b350ab4

remove useless diffs

7e776f6

improve conditional aesthetics

e46972d

move tests to top-level tests directory

b7155dd

pivnicek and others added 2 commits November 7, 2018 18:14

revert aesthetic conditional, sniff doesn't like it

7ea2ff4

Fix long conditional

a0cbdca

tomasfejfar force-pushed the piv-ci-and-cs branch from 45e8526 to 8c4b2c0 Compare November 8, 2018 10:07

Revert useless diffs

56b21b7

tomasfejfar force-pushed the piv-ci-and-cs branch from 8c4b2c0 to 56b21b7 Compare November 8, 2018 10:09

tomasfejfar added 5 commits November 8, 2018 11:10

fixup! Revert useless diffs

40330dd

fixup! Revert useless diffs

c711c84

fixup! Revert useless diffs

62a171f

fixup! Revert useless diffs

09d1203

fixup! Revert useless diffs

b80dfa6

pivnicek commented Nov 8, 2018

View reviewed changes

tomasfejfar added 2 commits November 8, 2018 11:18

fixup! Revert useless diffs

ff8ca40

reformat SQL

9c73355

tomasfejfar approved these changes Nov 8, 2018

View reviewed changes

get rid of debug dumps

dec8f9f

pivnicek mentioned this pull request Nov 8, 2018

7) Fix getTables request #75

Merged

pivnicek merged commit 4013ee4 into master Nov 12, 2018


		declare(strict_types=1);

		namespace Keboola\DbExtractor\Tests;

6) Update ci and cs #71

6) Update ci and cs #71

Conversation

pivnicek commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pivnicek commented Nov 7, 2018

pivnicek commented Nov 7, 2018

Choose a reason for hiding this comment

tomasfejfar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment