Update Sync.php #1086
Update Sync.php #1086
Conversation
app/Console/Commands/Sync.php
Outdated
| $exec->delete_imported = false; // we want to sync -> do not delete imported files | ||
| $exec->import_via_symlink = (Configs::get_value('import_via_symlink', '0') === '1'); | ||
| $exec->skip_duplicates = true; | ||
| $exec->delete_imported = $this->option('delete_imported'); |
There was a problem hiding this comment.
I recommend an improvement here in order to avoid coding the same error message "--delete_imported [...] and [...] are conflicting" twice.
First check if ($this->option('import_via_symlink') === '1' && $this->option('delete_imported') === '1') even before you set $exec->delete_imported (i.e. before line 41) and if the condition holds, then bail out with an error message. Then you can be sure, that both options are not set at the same time in the following. This also avoids the nested if below.
There was a problem hiding this comment.
Then you can be sure, that both options are not set at the same time in the following.
If we only check for options with if ($this->option('import_via_symlink') === '1' && $this->option('delete_imported')), how are we dealing with the Config setting (Configs::get_value('import_via_symlink', '0') === '1') == true and $this->option('delete_imported') ?
I've done it this way to make the sync backward compatible so that people using the config import_via_symlink === 1 don't have to add the --import_via_symlink=1 flag.
There was a problem hiding this comment.
Could you push your forked branch patch-1 from the repository pchampio/Lychee as a new branch to LycheeOrg/Lychee? However, I would recommend to rename the branch to something else than patch-1.
Then I could checkout the branch myself and push a commit to show what I mean. I think you don't get the idea.
There was a problem hiding this comment.
I took the liberty to push a new commit. I even went beyond my original idea and I added the additional feature that the default values of the command line options are taken from the user configuration.
There was a problem hiding this comment.
@pchampio What do you think?
There was a problem hiding this comment.
This is much better!
Thanks, I was looking for a way to set the default values of the command with reference to the configuration, but didn't found how to do it.
|
Pushing to a new branch on LycheeOrg is not feasible due to access right.
Télécharger BlueMail pour Android
|
Then I will do it. |
|
Kudos, SonarCloud Quality Gate passed!
|
|
LGTM, too. I would approve this PR, too, but it seems that I cannot as a push an own commit (?). I would recommend to wait another day to see whether @pchampio agrees with my changes. If we have no reply by then, we should merge this PR. |
fixes: #1056