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 lib/private/Lock #39108

Merged
merged 1 commit into from Sep 26, 2023

Conversation

fsamapoor
Copy link
Member

Summary

Following previous PRs taking advantage of PHP8's constructor property promotion in /core/ namespace, I have also made the required adjustments to the classes in /lib/private/Lock namespace.

The improvements in this PR include:

  • Using PHP8's constructor property promotion
  • Using match expression
  • Adding types to properties

Checklist

@solracsf solracsf added 3. to review Waiting for reviews technical debt labels Jul 3, 2023
@solracsf solracsf added this to the Nextcloud 28 milestone Jul 3, 2023
@szaimen szaimen requested review from rullzer, CarlSchwan, a team, ArtificialOwl, icewind1991 and Fenn-CS and removed request for a team July 12, 2023 11:48
Copy link
Contributor

@Fenn-CS Fenn-CS left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

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 dislike the match uses, I find them a lot less readable.

Also there is a problem with how the $ttl property is handled, see comments.

lib/private/Lock/DBLockingProvider.php Outdated Show resolved Hide resolved
lib/private/Lock/MemcacheLockingProvider.php Outdated Show resolved Hide resolved
@fsamapoor
Copy link
Member Author

I dislike the match uses, I find them a lot less readable.

Also there is a problem with how the $ttl property is handled, see comments.

Thank you, Come, for reviewing the PR. I really appreciate the timely feedback. I'm suggesting changes that I believe will improve code readability. However, I have no hard feelings if you prefer to revert or disregard them, as you are the maintainer.

Having said that, I've received multiple positive feedbacks in previous PRs regarding the use of match statements, and I personally try to minimize the use of else statements as much as I can.

I've moved the ttl property to the parent class. Please review the changes and let me know if I should revert the match statements.

@come-nc
Copy link
Contributor

come-nc commented Sep 26, 2023

Thank you, Come, for reviewing the PR. I really appreciate the timely feedback. I'm suggesting changes that I believe will improve code readability. However, I have no hard feelings if you prefer to revert or disregard them, as you are the maintainer.

Having said that, I've received multiple positive feedbacks in previous PRs regarding the use of match statements, and I personally try to minimize the use of else statements as much as I can.

I've moved the ttl property to the parent class. Please review the changes and let me know if I should revert the match statements.

Yes please revert the match statements. There is no need for minimizing the else statements, else is a perfectly fine language construct.
I only find match interesting for simple cases where an input value directly implies an output value. When there is more logic than that it quickly looks like perl oneliners.
And I especially find match(true) really non-intuitive.

To improve code readability.

Co-authored-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com>
Signed-off-by: Faraz Samapoor <f.samapoor@gmail.com>
Signed-off-by: Faraz Samapoor <fsa@adlas.at>
@fsamapoor
Copy link
Member Author

Thank you, Come, for reviewing the PR. I really appreciate the timely feedback. I'm suggesting changes that I believe will improve code readability. However, I have no hard feelings if you prefer to revert or disregard them, as you are the maintainer.
Having said that, I've received multiple positive feedbacks in previous PRs regarding the use of match statements, and I personally try to minimize the use of else statements as much as I can.
I've moved the ttl property to the parent class. Please review the changes and let me know if I should revert the match statements.

Yes please revert the match statements. There is no need for minimizing the else statements, else is a perfectly fine language construct. I only find match interesting for simple cases where an input value directly implies an output value. When there is more logic than that it quickly looks like perl oneliners. And I especially find match(true) really non-intuitive.

Done. Thank you for the explanation. Squashed the commits as well for a clean history.

@come-nc come-nc merged commit 740b38b into nextcloud:master Sep 26, 2023
33 of 38 checks passed
@fsamapoor fsamapoor deleted the refactor_lib_private_lock branch September 26, 2023 15:57
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