-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Test default value declaration for the date type #2851
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
Conversation
It was not covered, probably because it was hard to do something generic. I choose to duplicate some tests instead, so that things stay relatively simple.
d44f3e1
to
f1f728f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please open a PR with these small fixes?
'default' => $currentDateSql, | ||
); | ||
|
||
$this->assertEquals( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be self::assertSame()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is applicable on the other files as well...
); | ||
|
||
$this->assertEquals( | ||
' DEFAULT '.$currentDateSql, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is applicable on the other files as well...
public function testGetDefaultValueDeclarationSQLForDateType() | ||
{ | ||
$currentDateSql = $this->_platform->getCurrentDateSQL(); | ||
$field = array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use short-array syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is applicable on the other files as well...
@@ -563,6 +563,20 @@ public function testGetDefaultValueDeclarationSQLForIntegerTypes() | |||
} | |||
} | |||
|
|||
public function testGetDefaultValueDeclarationSQLForDateType() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testGetDefaultValueDeclarationSQLForDateType() : void
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is applicable on the other files as well...
@lcobucci will do, but it would be great to have an automatic check for this |
@greg0ire we're adding most of the checks on PHPCS but @Ocramius didn't like my PR that fixed all CS violations, so we had to make allowed to fail in travis https://travis-ci.org/doctrine/dbal/jobs/273964540 You can run |
I think that it could be possible to only check the new code, which @Ocramius would probably deem acceptable because it would allow slowly porting the code to the new style instead of creating a gazillion conflicts. |
I like chaos HAHAHAHA |
Could be achieved with something like https://github.com/greg0ire/git_template/blob/4877b9cc5786498eb89d2dfde6fc5c2e77e79c52/template/hooks/junkchecker/pre-commit#L18 , but I don't remember where I saw something like that with php-cs-fixer |
It was not covered, probably because it was hard to do something
generic. I choose to duplicate some tests instead, so that things stay
relatively simple.