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

Refactor userـldap app commands #39928

Merged
merged 2 commits into from
Feb 6, 2024

Conversation

fsamapoor
Copy link
Member

Summary

I have made some adjustments to the apps/user_ldap/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

Checklist

@fsamapoor fsamapoor force-pushed the refactor_user_ldap_app_commands branch from 2a9e9a7 to 9215858 Compare August 17, 2023 08:55
@fsamapoor fsamapoor added 3. to review Waiting for reviews technical debt labels Aug 22, 2023
@fsamapoor fsamapoor added this to the Nextcloud 28 milestone Aug 22, 2023
@fsamapoor fsamapoor requested review from a team, ArtificialOwl, blizzz and nfebe and removed request for a team August 22, 2023 06:31
Copy link
Contributor

@nfebe nfebe left a comment

Choose a reason for hiding this comment

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

Looks like some void-annotated methods do actually return values?

@fsamapoor
Copy link
Member Author

Looks like some void-annotated methods do actually return values?

Hello @fenn-cs, Thank you for reviewing the PR.

No method with the return type of void returns any values. The git diff seems like the return belongs to the method with a void return type, but it actually does not.

These two lines do not belong to the same method:

image

Here is the expanded diff:

image

Copy link
Contributor

@nfebe nfebe left a comment

Choose a reason for hiding this comment

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

Thanks Faraz!

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@blizzz blizzz force-pushed the refactor_user_ldap_app_commands branch from 9215858 to e46d750 Compare January 19, 2024 16:15
@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 19, 2024
@blizzz blizzz requested a review from come-nc January 19, 2024 16:15
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

I’m not sure why there is so much hate towards else statements, I find things clearer with them that without. Especially the continue in the loop instead of a simple else looks convoluted.
I like the strong typing, constructor promotion and constant usage in returns though.

Please revert at least the match (true).

apps/user_ldap/lib/Command/Search.php Outdated Show resolved Hide resolved
apps/user_ldap/lib/Command/UpdateUUID.php Outdated Show resolved Hide resolved
@fsamapoor fsamapoor force-pushed the refactor_user_ldap_app_commands branch from e46d750 to b7e19af Compare January 24, 2024 07:39
@fsamapoor
Copy link
Member Author

I’m not sure why there is so much hate towards else statements, I find things clearer with them that without. Especially the continue in the loop instead of a simple else looks convoluted. I like the strong typing, constructor promotion and constant usage in returns though.

Please revert at least the match (true).

Thank you for taking the time to review the changes Come. This was one of my older PRs and I appreciate your suggestions.

@come-nc come-nc force-pushed the refactor_user_ldap_app_commands branch from b7e19af to 30c0567 Compare January 30, 2024 08:36
apps/user_ldap/lib/Command/Search.php Outdated Show resolved Hide resolved
@come-nc come-nc force-pushed the refactor_user_ldap_app_commands branch from 2785151 to 46e2cef Compare February 1, 2024 16:39
@fsamapoor fsamapoor force-pushed the refactor_user_ldap_app_commands branch from 46e2cef to eca25b4 Compare February 5, 2024 08:03
fsamapoor and others added 2 commits February 6, 2024 10:02
To improve code readability.

Signed-off-by: Faraz Samapoor <f.samapoor@gmail.com>
Co-authored-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com>
Co-authored-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com>
Signed-off-by: Faraz Samapoor <f.samapoor@gmail.com>
@come-nc come-nc force-pushed the refactor_user_ldap_app_commands branch from eca25b4 to e6a4ebc Compare February 6, 2024 09:02
@come-nc come-nc merged commit ee6a62a into nextcloud:master Feb 6, 2024
133 of 140 checks passed
@fsamapoor fsamapoor deleted the refactor_user_ldap_app_commands branch February 6, 2024 12:40
@blizzz blizzz mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants