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

chore: unify __construct functions #2270

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Conversation

Chartman123
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@9729865). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2270   +/-   ##
=======================================
  Coverage        ?   45.05%           
  Complexity      ?      813           
=======================================
  Files           ?       68           
  Lines           ?     3072           
  Branches        ?        0           
=======================================
  Hits            ?     1384           
  Misses          ?     1688           
  Partials        ?        0           

@Chartman123 Chartman123 force-pushed the chore/unify_construct_functions branch from d82b036 to 68efc7f Compare August 8, 2024 22:20
Copy link
Collaborator

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Super nice refactoring!

@Chartman123 Chartman123 marked this pull request as ready for review August 14, 2024 12:20
@Chartman123
Copy link
Collaborator Author

I also tried to do it for the DB classes but got lots of errors so I left them as they are for now

Copy link
Collaborator

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Except for the whitespace issue this looks good!

Signed-off-by: Christian Hartmann <chris-hartmann@gmx.de>
@Chartman123 Chartman123 force-pushed the chore/unify_construct_functions branch from 68efc7f to c22c515 Compare August 14, 2024 12:48
@susnux susnux added the 3. to review Waiting for reviews label Aug 14, 2024
@Chartman123 Chartman123 merged commit 8410340 into main Aug 14, 2024
22 of 23 checks passed
@Chartman123 Chartman123 deleted the chore/unify_construct_functions branch August 14, 2024 13:37
@Chartman123
Copy link
Collaborator Author

@susnux I saw some errors like this one:

TypeError Cannot assign null to property OCA\Forms\Activity\ActivityManager::$currentUser of type OCP\IUser (OCA\Forms\Activity\ActivityManager->__construct())

It's coming from this line: $this->currentUser = $userSession->getUser(); (48) in ConfigService.php and ActivityManager.php

However I wasn't able to reliably reproduce it. Do you know how to fix it? I tried it with adding an ? to private IUser $currentUser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants