[Filesystem] [mirror] added "delete" option #6014

Closed
wants to merge 6 commits into
from

Projects

None yet

6 participants

@mylen
Contributor
mylen commented Nov 14, 2012

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.

@mylen mylen Filesystem: mirror with obsolete files removal in target dir
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
0fece92
@stof stof and 1 other commented on an outdated diff Nov 15, 2012
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 entries
+ if ($this->exists($targetDir)) {
+ if (isset($options['delete']) and $options['delete']) {
+ $l_iterator = $iterator;
@stof
stof Nov 15, 2012 Member

We are using camelCased names, not underscores. And why l ?

@mylen
mylen Nov 15, 2012 Contributor

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 ๐Ÿ‘

@stof
stof Nov 15, 2012 Member

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.

@mylen
mylen Nov 15, 2012 Contributor

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 entries
    
  •    if ($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/symfony/symfony/pull/6014/files#r2140156.

@mylen mylen modified variable name
$l_iterator  to $deleteIterator
61e2939
@pborreli pborreli and 2 others commented on an outdated diff Nov 16, 2012
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 entries
+ if ($this->exists($targetDir)) {
+ if (isset($options['delete']) and $options['delete']) {
@pborreli
pborreli Nov 16, 2012 Contributor

please replace and by &&

@mylen
mylen Nov 16, 2012 Contributor

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 ๐Ÿ‘

@schmittjoh
schmittjoh Nov 16, 2012 Contributor

and and && are not equivalent.

It doesn't matter in this case, but it could lead to weird behavior in other cases.

@mylen
mylen Nov 16, 2012 Contributor

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 :)

@mylen
mylen Nov 16, 2012 Contributor

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?

@mylen
mylen Nov 16, 2012 Contributor

in any case I would not trust precedence and add parenthesis... Hope it's coding standard too

@pborreli pborreli and 1 other commented on an outdated diff Nov 16, 2012
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 entries
+ if ($this->exists($targetDir)) {
+ if (isset($options['delete']) and $options['delete']) {
+ $deleteIterator = $iterator;
@pborreli
pborreli Nov 16, 2012 Contributor

why this assignation ?

@mylen
mylen Nov 16, 2012 Contributor

I want to make a copy of the incoming parameter, so I don't mess with the incoming param state during the deletion.
It's a very good question of you actually...

I want to iterate through the $targetDir and remove elements that are not in the $originDir.
Therefore, I have to use the CHILD_FIRST behavior (so that the branch is not deleted before the leaf)
I was wondering if the user would pass an $iterator as param, how do I make sure that he or she is going to provide an iterator with CHILD_FIRST?

In the unitary tests of the function, there is no test involving an $iterator as parameter so I don't know if my code is breaking anything...

By the way, do you have any comment on the unitary tests I have added?

@pborreli pborreli and 1 other commented on an outdated diff Nov 16, 2012
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 entries
+ if ($this->exists($targetDir)) {
+ if (isset($options['delete']) and $options['delete']) {
+ $deleteIterator = $iterator;
+ if (null === $deleteIterator) {
+ $flags = \FilesystemIterator::SKIP_DOTS;
+ $deleteIterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($targetDir, $flags), \RecursiveIteratorIterator::CHILD_FIRST );
@pborreli
pborreli Nov 16, 2012 Contributor

remove extra space before )

@mylen
mylen Nov 16, 2012 Contributor

done

@pborreli
Contributor

Symfony2 code standard use lowercase true and false

@mylen mylen coding standards
and => &&
FALSE => false
' )' => ')'
26dfbc3
@sstok sstok commented on an outdated diff Nov 16, 2012
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 entries
+ if ($this->exists($targetDir)) {
+ if (isset($options['delete']) && $options['delete']) {
@sstok
sstok Nov 16, 2012 Contributor

This can be merged with the above if () statement.

@mylen
Contributor
mylen commented Nov 16, 2012

I have problem to believe that the last commit (merging two if together) was to blame for the segfault on travis...
when I run the unit testing on my machine, I get:

I'm using PHP 5.4.7 by the way...

Time: 2 seconds, Memory: 3.25Mb

OK, but incomplete or skipped tests!
Tests: 80, Assertions: 106, Skipped: 14.

@pborreli
Contributor

Can you fix end-of-lines ?

@mylen
Contributor
mylen commented Nov 16, 2012

I put UNIX line feed and UTF8 charset

@mylen
Contributor
mylen commented Nov 16, 2012

Sorry, I add to clone the symfony repo with github and did small editing using pspad, I forgot to setup charset and line feed...

@fabpot fabpot added a commit that referenced this pull request Dec 11, 2012
@fabpot fabpot merged branch mylen/master (PR #6014)
This PR was squashed before being merged into the master branch (closes #6014).

Commits
-------

2b13760 [Filesystem] [mirror] added "delete" option

Discussion
----------

[Filesystem] [mirror] added "delete" option

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.

---------------------------------------------------------------------------

by pborreli at 2012-11-16T00:58:19Z

Symfony2 code standard use lowercase `true` and `false`

---------------------------------------------------------------------------

by mylen at 2012-11-16T20:25:19Z

I have problem to believe that the last commit (merging two if together) was to blame for the segfault on  travis...
when I run the unit testing on my machine, I get:

I'm using PHP 5.4.7 by the way...

Time: 2 seconds, Memory: 3.25Mb

OK, but incomplete or skipped tests!
Tests: 80, Assertions: 106, Skipped: 14.

---------------------------------------------------------------------------

by pborreli at 2012-11-16T20:38:40Z

Can you fix end-of-lines ?

---------------------------------------------------------------------------

by mylen at 2012-11-16T20:52:37Z

I put UNIX line feed and UTF8 charset

---------------------------------------------------------------------------

by mylen at 2012-11-16T20:53:59Z

Sorry, I add to clone the symfony repo with github and did small editing using pspad, I forgot to setup charset and line feed...
b3c58e7
@fabpot fabpot added a commit that closed this pull request Dec 11, 2012
@fabpot fabpot merged branch mylen/master (PR #6014)
This PR was squashed before being merged into the master branch (closes #6014).

Commits
-------

2b13760 [Filesystem] [mirror] added "delete" option

Discussion
----------

[Filesystem] [mirror] added "delete" option

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.

---------------------------------------------------------------------------

by pborreli at 2012-11-16T00:58:19Z

Symfony2 code standard use lowercase `true` and `false`

---------------------------------------------------------------------------

by mylen at 2012-11-16T20:25:19Z

I have problem to believe that the last commit (merging two if together) was to blame for the segfault on  travis...
when I run the unit testing on my machine, I get:

I'm using PHP 5.4.7 by the way...

Time: 2 seconds, Memory: 3.25Mb

OK, but incomplete or skipped tests!
Tests: 80, Assertions: 106, Skipped: 14.

---------------------------------------------------------------------------

by pborreli at 2012-11-16T20:38:40Z

Can you fix end-of-lines ?

---------------------------------------------------------------------------

by mylen at 2012-11-16T20:52:37Z

I put UNIX line feed and UTF8 charset

---------------------------------------------------------------------------

by mylen at 2012-11-16T20:53:59Z

Sorry, I add to clone the symfony repo with github and did small editing using pspad, I forgot to setup charset and line feed...
b3c58e7
@fabpot fabpot closed this in b3c58e7 Dec 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment