search

fiuba08 / robotframework

Automatically exported from code.google.com/p/robotframework
Apache License 2.0
0 stars 0 forks source link

OperatingSystem: New `Move/Copy Files` keywords for moving/copying multiple files (incl. wildcard support) #1613

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Currently the Move File and Copy File keywords of OperatingSystem library do 
not accept any wildcards to make it possible to copy or move multiple source 
files on one call. It would be nice to have also possibility to give a list of 
files to be copied to avoid using FOR-loops in a test case for that purpose.

Attached is a suggestion of the implementation (based on the library from 
RF2.8.3) and some test cases for testing the implementation and showing how the 
keywords work. I tested the implementation in Windows 7 with RF2.8.3 (Python), 
not in any other environments.

There is still anyhow at least one thing to decide: whether it should fail the 
keyword execution when multiple files are moved/copied to same destination file 
(i.e. when dest is not a directory but a file and there are multiple source 
files), or just warn about that and overwrite the destination file, like it 
does now.

The added methods are: copy_files, move_files, _normalize_dest and 
_prepare_list_of_source_files
Method _normalize_source_and_dest was modified to call _normalize_dest where 
part of the earlier content of that method was moved to.

Original issue reported on code.google.com by sami.sal...@gmail.com on 17 Dec 2013 at 7:03

Attachments:

GoogleCodeExporter commented 9 years ago 
I forgot to finalize the implementation suggestion before attaching the module 
to the issue. In the attached module, the move_file and copy_file methods are 
used for the actual moving/copying after the list of files is ready.

Original comment by sami.sal...@gmail.com on 18 Dec 2013 at 7:51

Attachments:

GoogleCodeExporter commented 9 years ago 
1) I agree Move/Copy Files keywords, with wildcard support, would be good 
additions to OperatingSystem. Including this tentatively to 2.8.4.

2) To make it easier to see your changes, it would be great if you could attach 
a patch containing only the relevant changes. If you have checked out the code, 
you can run e.g. `hg diff > move_copy_file.patch` to create one. If you use a 
graphical version control client, you need to search the docs of you tool to 
find out how to create patch.

3) Trying to copy/move multiple files into same destination fails should be an 
error.

4) Are you interested in turning your example tests to real acceptance tests 
for this functionality? To get an idea how Robot's own acceptance tests work, 
first take a look at atest/README.txt file in the project repository. I can 
obviously also help you with this task.

Original comment by pekka.klarck on 18 Dec 2013 at 10:29

GoogleCodeExporter commented 9 years ago 
Attached is a patch file. I implemented acceptance tests also as can be seen 
from the patch file.

Trying to copy/move multiple files into same destination file now fails the 
keyword execution.

Original comment by sami.sal...@gmail.com on 19 Dec 2013 at 4:37

Attachments:

GoogleCodeExporter commented 9 years ago

Original comment by mikko.ko...@gmail.com on 24 Jan 2014 at 10:11

GoogleCodeExporter commented 9 years ago 
@sami.sallmen: Thanks for the patch! Applied in r566c4bc2f2d0.

I also changed the order of arguments so that sources will be before the 
destination.
This was done in r283e99efe57c

Original comment by mikko.ko...@gmail.com on 24 Jan 2014 at 12:30

GoogleCodeExporter commented 9 years ago 
Still I think the destination should always be a directory - we need to discuss 
this a bit with the team.

Original comment by mikko.ko...@gmail.com on 24 Jan 2014 at 12:35

GoogleCodeExporter commented 9 years ago 
My original use case for needing the move/copy file keywords with wildcard 
support was to rename a log file from an external test tool. I don't know the 
full name of the log file when running the test, only the beginning of the file 
name is known and the rest of the name changes every time. So, if the 
destination would be a directory, I still wouldn't know the name of the file to 
analyze its contents and the keyword would not help me at all in the original 
problem. Please do not restrict it so that the destination must be a directory.

Original comment by sami.sal...@gmail.com on 24 Jan 2014 at 4:26

GoogleCodeExporter commented 9 years ago 
We discussed this and agreed that the destination needs to be a directory. Some 
reasons for this decision:

1) That's what mv/cp commands require also when moving/copying multiple files.

2) If the destination does not exist, it would be impossible to know does the 
user want the destination to be a directory or file. Guessing based on possible 
extension is not a solution.

3) If multiple source files are moved/copied so that the destination is a 
single file, the effect would be that all but the last of the source files 
would be deleted. Possibility for accidental data-loss is a big no-no.

We also discussed about the use case of renaming a file which name is not fully 
known. We agreed that the use case is valid, but that doesn't change the fact 
that the proposed Move/Copy Files keywords could support it. Adding this 
support for the existing Move/Copy File keywords would be possible, though. It 
ought to be relatively easy to change them so that the source can specified as 
a glob pattern that must match exactly one file. Please submit a separate issue 
if you think it is a good idea.

Original comment by pekka.klarck on 27 Jan 2014 at 10:38

GoogleCodeExporter commented 9 years ago 
Ok, fair enough. I will anyhow keep my own implementation of Move/Copy Files 
keywords in my own library as the new keywords do not serve me at all now. 
Thus, I don't need any changes to the existing Move Copy File keywords either.

Original comment by sami.sal...@gmail.com on 27 Jan 2014 at 10:55

GoogleCodeExporter commented 9 years ago 
BTW, only the first reason (mv/cp requires dir as dest when multiple sources) 
is applicable, but the two other comments are not, what comes to my 
implementation.

2) The implementation checks if the destination path ends with a path separator 
and determines if the destination is a directory or not based on that. So, it 
is not at all impossible to know if the user wants the destination to be a file 
or directory.

3) If there are multiple source files and the destination is one file, the 
keyword fails. So, no data loss at all.

If you read (and understand) the source code and/or the test cases, you will 
notice all that. I did not write that into the keyword documentation as I 
didn't want to make it too long. Both of these features could be of course 
explained in the keyword documentation also.

Original comment by sami.sal...@gmail.com on 28 Jan 2014 at 7:46

GoogleCodeExporter commented 9 years ago 
I didn't actually look at the code but obviously knew 2) and 3) could be 
addressed. The main problem would have been, as you also noticed, documenting 
the functionality so that others would have been able to use it. Requiring 
users to add a path separator when they want to move/copy files to a 
non-existing directory would have also been somewhat annoying, especially when 
that is one of the main use cases of these keywords. Finally, adding special 
features for moving/copying a single file to a keyword that is designed to move 
multiple files would just be a bit odd.

As I already noted, this functionality to move/copy single not-fully-known file 
would very well fit into to the existing Move/Copy File keywords. It wouldn't 
make any other use case harder and documenting the functionality would also be 
pretty simple. We'd just need to add a not like "The source can also be a glob 
pattern that matches exactly one file. This allows moving/copying files whose 
name is not fully known." Anyway, adding that functionality requires a separate 
issue.

Original comment by pekka.klarck on 28 Jan 2014 at 5:54

GoogleCodeExporter commented 9 years ago 
Issue 1639 created for adding wildcard functionality to Move/Copy File keywords.

Original comment by sami.sal...@gmail.com on 28 Jan 2014 at 10:15

GoogleCodeExporter commented 9 years ago 
Docs ready after r59d5d847e628.
The implementation done in:
r104b49e39ba5
r8d53dfa1e5cd
r19e9d9e04dde
r283e99efe57c

To Review

Original comment by mikko.ko...@gmail.com on 30 Jan 2014 at 12:30

GoogleCodeExporter commented 9 years ago 
Looks good. Some cleanup/enhancements to tests and docs in revision 
3a1ddc2ab95f and revision 0e012ec1abf1, respectively.

Original comment by pekka.klarck on 30 Jan 2014 at 2:54