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

Stricter typing to match psalm level 2 #4584

Merged
merged 4 commits into from Jul 27, 2023
Merged

Stricter typing to match psalm level 2 #4584

merged 4 commits into from Jul 27, 2023

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Jul 24, 2023

Did some work towards higher psalm levels during todays travel, let's see what CI thinks about that

  • Check why psalm locally complains about stuff from vendor/nextcloud/ocp which should only be additional files
    • No idea why this happens but added to the baseline for now as it is not text related

@cypress
Copy link

cypress bot commented Jul 24, 2023

Passing run #11337 ↗︎

0 149 2 0 Flakiness 0

Details:

Stricter typing to match psalm level 2
Project: Text Commit: 7b24775f17
Status: Passed Duration: 03:45 💡
Started: Jul 27, 2023 6:32 PM Ended: Jul 27, 2023 6:35 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@juliushaertl juliushaertl force-pushed the chore/stricter branch 2 times, most recently from 9fe178a to 1b300ce Compare July 24, 2023 15:42
@juliushaertl juliushaertl marked this pull request as ready for review July 25, 2023 12:05
@juliushaertl juliushaertl requested review from mejo- and max-nextcloud and removed request for mejo- July 25, 2023 12:05
@juliushaertl juliushaertl added this to the Nextcloud 28 milestone Jul 25, 2023
@juliushaertl juliushaertl changed the title chore/stricter Stricter typing to match psalm level 2 Jul 25, 2023
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Thanks a lot for looking into this 🐝

Apart from two small comments changes all look sensible to me 😊

@@ -77,6 +77,7 @@ private function detectUtfBom(string $string): ?string {
*/
private function getEncodings(): array {
$mbOrder = mb_detect_order() ?: [];
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this line can go, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

The return value is still needed, unfortunately the return value of mb_detect_order is strange as it is documented to return a bool|array while when calling it without an argument it will always return an array. I've updated the relevant line to require the duplicate variable assignment

@@ -127,6 +124,8 @@ public function create($fileId = null, $filePath = null, ?string $token = null,
return new DataResponse('Failed to create the document session', 500);
}

/** @var Document $document */

Copy link
Member

Choose a reason for hiding this comment

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

Nitpicking, but this empty line is superfluous, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not the var of the left line but an indicator for psalm not detecting that document is never undefined in this case (which seems like a false positive).

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl juliushaertl merged commit a235396 into main Jul 27, 2023
39 checks passed
@juliushaertl juliushaertl deleted the chore/stricter branch July 27, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants