Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test for null byte #56

Merged
merged 5 commits into from Oct 5, 2018
Merged

test for null byte #56

merged 5 commits into from Oct 5, 2018

Conversation

tomasfejfar
Copy link
Contributor

No description provided.

@pivnicek pivnicek self-requested a review October 5, 2018 16:21
@@ -55,6 +56,18 @@ public function testConnection(): void
$this->db->query('SELECT GETDATE() AS CurrentDateTime')->execute();
}

private function stripNulls(string $fileName): void
{
$process = new Process(sprintf('sed -e \'s/,\x00,/,,/\' -i %s', $fileName));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would not trust myself with escaping, which is why I would use argument array

// traditional string based commands
$builder = new Process('ls -lsa');
// same example but using an array
$builder = new Process(array('ls', '-lsa'));
// the array can contain any number of arguments and options
$builder = new Process(array('ls', '-l', '-s', '-a'));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also definitely needs a comment to explain what it does and why

@@ -55,6 +56,18 @@ public function testConnection(): void
$this->db->query('SELECT GETDATE() AS CurrentDateTime')->execute();
}

private function stripNulls(string $fileName): void
Copy link
Contributor Author

Choose a reason for hiding this comment

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

stripNullBytesInEmptyFields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ideally move outside the class and test it in isolation

Copy link
Contributor Author

@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.

LGTM, but cant approve because i opened the PR :)

Copy link
Contributor

@pivnicek pivnicek left a comment

Choose a reason for hiding this comment

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

;p

@pivnicek pivnicek merged commit b24e04d into master Oct 5, 2018
@pivnicek pivnicek deleted the piv-strip-nulls branch October 5, 2018 17:30
@tomasfejfar tomasfejfar mentioned this pull request Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants