Skip to content

Add template typing to the QBMapper - #23310

Merged
MorrisJobke merged 2 commits into
masterfrom
enhancement/psalm-typed-qb-mapper
Oct 12, 2020
Merged

Add template typing to the QBMapper#23310
MorrisJobke merged 2 commits into
masterfrom
enhancement/psalm-typed-qb-mapper

Conversation

@ChristophWurst

Copy link
Copy Markdown
Member

The QBMapper is kind of a generic type, though this concept does not
exist in php. Hence you have a lot of type coercion in subtypes (mappers
in the individual apps) because you suddenly don't expect an Entity[]
but your specific type.

Luckily Psalm lets us type those. Then in the subclass you can
psalm-implement the mapper with a concrete type and psalm will do all
the magic to ensure types are used correctly.

@ChristophWurstChristophWurst added the 2. developing Work in progress label Oct 9, 2020
@ChristophWurstChristophWurst added this to the Nextcloud 21 milestone Oct 9, 2020
@ChristophWurstChristophWurst self-assigned this Oct 9, 2020
@ChristophWurstChristophWurst added 3. to review Waiting for reviews enhancement and removed 2. developing Work in progress labels Oct 9, 2020
@ChristophWurst
ChristophWurst marked this pull request as ready for review October 9, 2020 12:33
@ChristophWurst

Copy link
Copy Markdown
MemberAuthor

nextcloud/mail#3783 is how I discovered that this actually works. Requires error level 3 or lower to show the type coercion. With the template I could resolve the issues :)

Call me crazy but how about backporting this? It's not a functional change, just a different way to describe the API. Objections?

@ChristophWurst
ChristophWurstforce-pushed the enhancement/psalm-typed-qb-mapper branch from d24a1b1 to b0a956eCompareOctober 9, 2020 12:40

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice 👍

@kesselb

This comment has been minimized.

The QBMapper is kind of a generic type, though this concept does not
exist in php. Hence you have a lot of type coercion in subtypes (mappers
in the individual apps) because you suddenly don't expect an Entity[]
but your specific type.
Luckily Psalm lets us type those. Then in the subclass you can
psalm-implement the mapper with a concrete type and psalm will do all
the magic to ensure types are used correctly.
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
ChristophWurstforce-pushed the enhancement/psalm-typed-qb-mapper branch from b0a956e to 2664c7fCompareOctober 12, 2020 09:10
@ChristophWurst

Copy link
Copy Markdown
MemberAuthor

/backport a1b3510 to stable20

@ChristophWurst

Copy link
Copy Markdown
MemberAuthor

/backport a1b3510 to stable19

@ChristophWurstChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 12, 2020
@faily-bot

faily-botBot commented Oct 12, 2020

Copy link
Copy Markdown

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 33975: failure

sqlite

Show full log
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There was 1 failure:
1) TrashbinTest::testExpireOldFilesShared
Failed asserting that 3 is identical to 2.
/drone/src/apps/files_trashbin/tests/TrashbinTest.php:304
/drone/src/apps/files_trashbin/tests/TrashbinTest.php:287

mariadb10.1-php7.3

mariadb10.4-php7.4

mysql8.0-php7.4

mysql5.6-php7.3

postgres9-php7.3

Show full log
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There was 1 failure:
1) TrashbinTest::testExpireOldFilesShared
Failed asserting that 3 is identical to 2.
/drone/src/apps/files_trashbin/tests/TrashbinTest.php:304
/drone/src/apps/files_trashbin/tests/TrashbinTest.php:287

postgres11-php7.4

@ChristophWurst

Copy link
Copy Markdown
MemberAuthor

Failing tests can only be unrelated as the comments only regard psalm and that passes 😇

@MorrisJobke
MorrisJobke merged commit b6364cc into masterOct 12, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishenhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ChristophWurst@kesselb@MorrisJobke