[Filesystem] [mirror] added "delete" option #6014
Conversation
I added a "delete" option to the mirror function. If set to TRUE, then files present in target dir and not in origin dir will be removed
| // Iterate in destination folder to remove obsolete entries | ||
| if ($this->exists($targetDir)) { | ||
| if (isset($options['delete']) and $options['delete']) { | ||
| $l_iterator = $iterator; |
There was a problem hiding this comment.
We are using camelCased names, not underscores. And why l ?
There was a problem hiding this comment.
I see your point, I didn't want to overwrite the $iterator variable so I had to use another name... the 'l' stands for 'local' I know it's unusual, I learned to use 'l_' at school and it's coming back from time to time... I will change it if you want 👍
There was a problem hiding this comment.
Well, the variable name should follow the coding standards used in the project, and should be understandable by people who went to a different school than you 😄
So yeah, please rename it.
There was a problem hiding this comment.
Ok, done! I tried to push the change from work, but could not manage to
have git sending anything behind http proxy :(
On 15 November 2012 16:51, Christophe Coevoet notifications@github.comwrote:
In src/Symfony/Component/Filesystem/Filesystem.php:
* * @throws IOException When file type is unknown */ public function mirror($originDir, $targetDir, \Traversable $iterator = null, $options = array()) {
$targetDir = rtrim($targetDir, '/\');$originDir = rtrim($originDir, '/\');
// Iterate in destination folder to remove obsolete entriesif ($this->exists($targetDir)) {if (isset($options['delete']) and $options['delete']) {$l_iterator = $iterator;Well, the variable name should follow the coding standards used in the
project, and should be understandable by people who went to a different
school than you [image: 😄]
So yeah, please rename it.—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6014/files#r2140156.
$l_iterator to $deleteIterator
|
|
||
| // Iterate in destination folder to remove obsolete entries | ||
| if ($this->exists($targetDir)) { | ||
| if (isset($options['delete']) and $options['delete']) { |
There was a problem hiding this comment.
I thought I good get rid of the && with an high level language :) At least with the 'and' your are not likely to put a '&' by mistake... But if it's standard thanks for the tips 👍
There was a problem hiding this comment.
and and && are not equivalent.
It doesn't matter in this case, but it could lead to weird behavior in other cases.
There was a problem hiding this comment.
OMG, I didn't check that... I'm glad I post that PR, I'm learning a great deal of stuff !!
let's google now so I can check the diff :)
There was a problem hiding this comment.
Ok, I get it that the and as less precedence than the && and if I try:
$buf = 1;
$val = ($buf += 2 && $buf += 2);
$val is false
and
$buf = 1;
$val = ($buf += 2 and $buf += 2);
$val is true
Am I right?
There was a problem hiding this comment.
in any case I would not trust precedence and add parenthesis... Hope it's coding standard too
|
Symfony2 code standard use lowercase |
and => && FALSE => false ' )' => ')'
|
|
||
| // Iterate in destination folder to remove obsolete entries | ||
| if ($this->exists($targetDir)) { | ||
| if (isset($options['delete']) && $options['delete']) { |
There was a problem hiding this comment.
This can be merged with the above if () statement.
|
I have problem to believe that the last commit (merging two if together) was to blame for the segfault on travis... I'm using PHP 5.4.7 by the way... Time: 2 seconds, Memory: 3.25Mb OK, but incomplete or skipped tests! |
|
Can you fix end-of-lines ? |
|
I put UNIX line feed and UTF8 charset |
|
Sorry, I add to clone the symfony repo with github and did small editing using pspad, I forgot to setup charset and line feed... |
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets:
Todo: -
License of the code: MIT
Documentation PR: symfony/symfony-docs#123
I added a "delete" option to the mirror function. If set to true, then
files present in target dir and not in origin dir will be removed.
Added also unit test for these feature.