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

Refactors files app commands #39150

Merged
merged 4 commits into from Aug 3, 2023

Conversation

fsamapoor
Copy link
Member

I have made some adjustments to the apps/files/lib/Command classes to improve the code readability.

The improvements in this PR include but are not limited to:

  • Using PHP8's constructor property promotion
  • Adding return types
  • Replacing return code integers with more readable strings.
  • Using early returns

@fsamapoor fsamapoor changed the title Refactor files app commands Refactors files app commands Jul 4, 2023
@solracsf solracsf added 3. to review Waiting for reviews technical debt labels Jul 5, 2023
@solracsf solracsf added this to the Nextcloud 28 milestone Jul 5, 2023
@solracsf
Copy link
Member

solracsf commented Jul 5, 2023

Psaml still failing with:

Error: apps/files/lib/Command/ScanAppData.php:170:21: InvalidArgument: Argument 1 of set_error_handler expects callable(int, string, string=, int=, array<array-key, mixed>=):bool|null, but list{OCA\Files\Command\ScanAppData&static, 'exceptionErrorHandler'} provided

@fsamapoor
Copy link
Member Author

Psaml still failing with:

Error: apps/files/lib/Command/ScanAppData.php:170:21: InvalidArgument: Argument 1 of set_error_handler expects callable(int, string, string=, int=, array<array-key, mixed>=):bool|null, but list{OCA\Files\Command\ScanAppData&static, 'exceptionErrorHandler'} provided

@solracsf I was searching for the solution since it seems to be a Psalm false positive error.

It does have a hacky solution indeed that I didn't like to be quite honest.

In the end, I figured to just remove the return type from the method, as there are multiple similar methods in the codebase that do not have a return type and this way, the codebase remains consistent regarding the issue.
Anyways, all good now. Thanks for reminding me.

@szaimen szaimen requested review from icewind1991, artonge, come-nc, a team and ArtificialOwl and removed request for a team July 12, 2023 12:04
@artonge artonge force-pushed the refactor_files_app_commands branch from 636e740 to fcd6f4e Compare July 12, 2023 14:01
To improve code readability.

Signed-off-by: Faraz Samapoor <fsa@adlas.at>
Signed-off-by: Faraz Samapoor <fsa@adlas.at>
I just removed the return type from the "exceptionErrorHandler". There is in fact a hacky solution, that I didn't like:
vimeo/psalm#3571

Signed-off-by: Faraz Samapoor <fsa@adlas.at>
@fsamapoor fsamapoor force-pushed the refactor_files_app_commands branch from fcd6f4e to 270035f Compare July 12, 2023 15:00
apps/files/lib/Command/Scan.php Fixed Show fixed Hide fixed
apps/files/lib/Command/Scan.php Fixed Show fixed Hide fixed
apps/files/lib/Command/Scan.php Fixed Show fixed Hide fixed
apps/files/lib/Command/Scan.php Fixed Show fixed Hide fixed
apps/files/lib/Command/Scan.php Fixed Show fixed Hide fixed
@fsamapoor
Copy link
Member Author

Resolving conflicts with web ide messed things up. Gonna fix the issue and report back ASAP.

Signed-off-by: Faraz Samapoor <f.samapoor@gmail.com>
@fsamapoor
Copy link
Member Author

Resolving conflicts with web ide messed things up. Gonna fix the issue and report back ASAP.

Fixed.

@come-nc come-nc merged commit 26049cb into nextcloud:master Aug 3, 2023
35 of 39 checks passed
@fsamapoor fsamapoor deleted the refactor_files_app_commands branch August 3, 2023 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants