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.

Sign up for GitHub

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

Jump to bottom

Add missing instanceof check #4401

Merged
merged 1 commit into from Nov 3, 2020
Merged

Add missing instanceof check #4401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions psalm.xml
View file
Open in desktop
Expand Up @@ -89,6 +89,15 @@
<file name="src/Schema/AbstractSchemaManager.php"/>
</errorLevel>
</PossiblyNullArgument>
<RedundantCondition>
<errorLevel type="suppress">
<!--
Requires a release of
https://github.com/sebastianbergmann/phpunit/commit/9c60d7d9fd3bfa80fa4aeab7090e1bbe0830dbcd
Copy link
Member

Choose a reason for hiding this comment

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

In the future, I believe it's better to identify the issue by the GitHub issue rather than a commit. Otherwise, it requires extra navigation which may not always work (e.g. if commit gets rebased or processed in some other way).

Copy link
Member

Choose a reason for hiding this comment

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

I advise this because the commit view allows to see which tags/branches it is on. I wonder if that works in case of "Rebase and merge" / "squash"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can always navigate from issue to commit, not sure the opposite is true?

Copy link
Member

Choose a reason for hiding this comment

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

In most cases you can, I'm not sure if there are exceptions. Anyway, the PR does not contain information about branches and tags, only the commit does AFAIK

Copy link
Member

@morozov morozov Nov 4, 2020 •
edited

Choose a reason for hiding this comment

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

The exception is when a PR gets rebased (effectively, excluded from the tree). If the navigation works from the commit to the PR, it will work the other way around as well. At the same time, a PR or a commit not always exists (it can be just an issue), so from the consistency standpoint, it's better to link to issues and PRs instead of issues and commits. Additionally, issues usually contain more context, discussions, labels, etc.

-->
<file name="tests/Driver/API/ExceptionConverterTest.php"/>
</errorLevel>
</RedundantCondition>
<TooFewArguments>
<errorLevel type="suppress">
<!--
Expand Down
1 change: 1 addition & 0 deletions tests/Driver/API/ExceptionConverterTest.php
View file
Open in desktop
Expand Up @@ -51,6 +51,7 @@ public function testConvertsException(

$dbalException = $this->converter->convert($driverException, $query);

self::assertInstanceOf($expectedClass, $dbalException);
self::assertSame($driverException->getCode(), $dbalException->getCode());
self::assertSame($driverException->getSQLState(), $dbalException->getSQLState());
self::assertSame($driverException, $dbalException->getPrevious());
Expand Down