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.

Sign up for GitHub

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

Jump to bottom

Refactor encryption app commands #39692

Merged
merged 2 commits into from
Aug 16, 2024
Merged

Refactor encryption app commands #39692

merged 2 commits into from
Aug 16, 2024

Conversation

fsamapoor
Copy link
Member

@fsamapoor fsamapoor commented Aug 3, 2023 •
edited
Loading

Summary

I have made some adjustments to the apps/encryption/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_encryption_app_commands branch from 1553fcb to 4507123 Compare August 3, 2023 11:31
@fsamapoor fsamapoor requested review from a team, ArtificialOwl, icewind1991 and Fenn-CS and removed request for a team August 5, 2023 20:25
@fsamapoor fsamapoor added 3. to review Waiting for reviews technical debt labels Aug 5, 2023
@fsamapoor fsamapoor force-pushed the refactor_encryption_app_commands branch from 4507123 to 1b009e2 Compare August 10, 2023 11:52
@fsamapoor
Copy link
Member Author

Conflicts resolved.

@fsamapoor fsamapoor added this to the Nextcloud 28 milestone Aug 10, 2023
Fenn-CS
Fenn-CS approved these changes Aug 14, 2023
View reviewed changes
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.

Please, rebase this against latest master.

@fsamapoor fsamapoor force-pushed the refactor_encryption_app_commands branch from 1b009e2 to 881123f Compare August 14, 2023 08:51
@fsamapoor
Copy link
Member Author

Please, rebase this against latest master.

Done. But it has already got out-of-dated. Should I rebase it again?
I think we can wait for the second review for now.

@Fenn-CS
Copy link
Contributor

Fenn-CS commented Aug 14, 2023

@fsamapoor thanks, all good! I was hoping CI might get green if you rebase but no, it's fine. Just wait for other reviews.

@fsamapoor fsamapoor force-pushed the refactor_encryption_app_commands branch from 881123f to ab2caed Compare August 15, 2023 06:17
@fsamapoor
Copy link
Member Author

@Fenn-CS Cypress workflows are failing on almost all of my PRs. I don't know if there is anything that I can do to fix them.

@Fenn-CS
Copy link
Contributor

Fenn-CS commented Aug 17, 2023

@fsamapoor Not to worry, those failures are unrelated (most likely as they are UI tests).

@fsamapoor fsamapoor force-pushed the refactor_encryption_app_commands branch from ab2caed to 37bf803 Compare September 9, 2023 06:41
@fsamapoor
Copy link
Member Author

Conflicts resolved.

@Fenn-CS
Copy link
Contributor

Fenn-CS commented Sep 12, 2023

Conflicts resolved.

Thank you, please rebase.

@fsamapoor fsamapoor force-pushed the refactor_encryption_app_commands branch from 37bf803 to a45dd55 Compare September 12, 2023 11:57
@fsamapoor
Copy link
Member Author

Thank you, please rebase.

Done.

@Fenn-CS
Copy link
Contributor

Fenn-CS commented Sep 18, 2023

@fsamapoor now that this has another approval kindly, update/rebase it with master I would put my eyes on the jobs to get it in asap.

Thank you!

@fsamapoor fsamapoor force-pushed the refactor_encryption_app_commands branch from a45dd55 to 68da42a Compare September 18, 2023 19:10
@fsamapoor
Copy link
Member Author

@fsamapoor now that this has another approval kindly, update/rebase it with master I would put my eyes on the jobs to get it in asap.

Thank you!

Sure. Thank you! Other than Cypress workflows, I think there will be no issues.

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
Merged
This was referenced Mar 14, 2024
Merged
Merged
Merged
@Altahrim Altahrim mentioned this pull request Mar 25, 2024
Merged
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
Merged
81 tasks
This was referenced Apr 4, 2024
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 29, Nextcloud 30 Apr 8, 2024
@skjnldsv skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Jul 27, 2024
This was referenced Jul 30, 2024
Merged
Merged
This was referenced Aug 5, 2024
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
Merged
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
@skjnldsv skjnldsv reopened this Aug 16, 2024
Faraz Samapoor added 2 commits August 16, 2024 09:33
@skjnldsv
Refactors encryption app commands.
fc8b886 
To improve code readability.

Signed-off-by: Faraz Samapoor <fsa@adlas.at>
@skjnldsv
Uses early returns.
6b795da 
To improve code readability.

Signed-off-by: Faraz Samapoor <fsa@adlas.at>
@skjnldsv skjnldsv force-pushed the refactor_encryption_app_commands branch from 7899057 to 6b795da Compare August 16, 2024 07:33
@skjnldsv skjnldsv self-assigned this Aug 16, 2024
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress stale Ticket or PR with no recent activity labels Aug 16, 2024
@skjnldsv skjnldsv merged commit 75180a6 into nextcloud:master Aug 16, 2024
155 of 161 checks passed
@skjnldsv skjnldsv added this to the Nextcloud 31 milestone Aug 16, 2024
@fsamapoor fsamapoor deleted the refactor_encryption_app_commands branch August 19, 2024 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@icewind1991 icewind1991 icewind1991 approved these changes

@Fenn-CS Fenn-CS Fenn-CS approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

Assignees

@skjnldsv skjnldsv

Labels
4. to release Ready to be released and/or waiting for tests to finish technical debt
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.
5 participants
@fsamapoor @Fenn-CS @icewind1991 @blizzz @skjnldsv