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

No issue - Refactor code related to backuppers and restorers #100

Merged
merged 11 commits into from
Feb 26, 2024

Conversation

Lonnytunes
Copy link
Collaborator

No description provided.

@Lonnytunes Lonnytunes self-assigned this Feb 23, 2024
@Lonnytunes Lonnytunes force-pushed the no-issue-backuppers-restorers-refactoring branch from 26e1ab9 to 370e7ee Compare February 23, 2024 15:53
@Lonnytunes
Copy link
Collaborator Author

@pounard @SimonMellerin

I created the directory src/Utility in a previous PR for the CommandLine class.

I added AbstractProcessTrait (to rename?) and ChainLogger elements into this directory for now.

But I think CommandLine and AbstractProcessTrait could be in an src/Process directory, and ChainLogger in an src/Log.

Let me know your opinions! 🙂

@Lonnytunes Lonnytunes force-pushed the no-issue-backuppers-restorers-refactoring branch from 370e7ee to 6a4cbf3 Compare February 23, 2024 16:14
@Lonnytunes Lonnytunes force-pushed the no-issue-backuppers-restorers-refactoring branch from 6a4cbf3 to 89762b4 Compare February 23, 2024 16:19
src/Backupper/AbstractBackupper.php Outdated Show resolved Hide resolved
src/Backupper/BackupperFactory.php Show resolved Hide resolved
src/Command/Anonymization/AnonymizeCommand.php Outdated Show resolved Hide resolved
src/Command/BackupCommand.php Outdated Show resolved Hide resolved
@pounard
Copy link
Member

pounard commented Feb 26, 2024

@Lonnytunes

But I think CommandLine and AbstractProcessTrait could be in an src/Process directory, and ChainLogger in an src/Log.

I have no strong opinion about this. For the first part, the Process namespace is a good idea, for the other one, I think that the Utility namespace must be removed, and the ChainLogger moved into the Helper namespace, which is basically the same.

Let's keep only one "fourzytou" namespace (sorry, french joke, but you get what I mean).

I'do go further and place the Process namespace under Helper, such as this: src/Helper/Process/....

@Lonnytunes
Copy link
Collaborator Author

Lonnytunes commented Feb 26, 2024

Thanks for your answer @pounard

I will move ChainLogger from the Utility namespace to the Helper one. It's actually a kind of duplicate.

But I prefer to keep the idea of the Process namespace at the root for now.

Are you agree to rename AbstractProcessTrait to ProcessTrait?

@pounard
Copy link
Member

pounard commented Feb 26, 2024

Are you agree to rename AbstractProcessTrait to ProcessTrait?

Yes, it's either abstract, either a trait, but not both.

@Lonnytunes Lonnytunes force-pushed the no-issue-backuppers-restorers-refactoring branch from 8e26866 to d0b928f Compare February 26, 2024 13:00
@Lonnytunes Lonnytunes force-pushed the no-issue-backuppers-restorers-refactoring branch from d0b928f to 6c7ad0a Compare February 26, 2024 13:13
@Lonnytunes Lonnytunes force-pushed the no-issue-backuppers-restorers-refactoring branch from 485b201 to 8e6ff77 Compare February 26, 2024 13:38
src/Backupper/AbstractBackupper.php Outdated Show resolved Hide resolved
src/Restorer/AbstractRestorer.php Outdated Show resolved Hide resolved
src/Restorer/AbstractRestorer.php Show resolved Hide resolved
@Lonnytunes Lonnytunes marked this pull request as ready for review February 26, 2024 14:27
@Lonnytunes Lonnytunes merged commit 2933a28 into main Feb 26, 2024
25 checks passed
@Lonnytunes Lonnytunes deleted the no-issue-backuppers-restorers-refactoring branch February 26, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants