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

Add copy method to aruba #219

Merged
merged 6 commits into from Jun 10, 2015
Merged

Add copy method to aruba #219

merged 6 commits into from Jun 10, 2015

Conversation

ghost
Copy link

@ghost ghost commented Oct 16, 2014

In one of my projects I need to copy files around during testing. I added a method to do this.

@jarl-dk Would you mind to review this?

@ghost
Copy link
Author

ghost commented Nov 25, 2014

This one needs to be discussed first... It might be helpfull to copy files from outside to current_dir and not within current_dir. A second opinion would be great.

@jarl-dk
Copy link
Member

jarl-dk commented Dec 10, 2014

I am open for adding a copy method to the API, but I think we should stick to a similar interface as FileUtils#cp_r. So if you could please update the PR (or make another one) to not use the splat operator (still allowing a list as first argument), then I think this will be a great improvement to Aruba.

Maybe the method name shold be copy_files (indicating an array is possible as first argument)

Please also see #218 (comment)

@ghost
Copy link
Author

ghost commented Dec 10, 2014

Ok. I see. I tried to mimic the original programs.

touch 1 2 3 4 on commandline will create all four files. cp -r 1 2 3 4 dst/ will copy all four files to dst.

@ghost
Copy link
Author

ghost commented Dec 10, 2014

One question... What do you think...

a) Source files should be expanded to current_dir?
b) Destination files should be expanded to current_dir?
c) Both - source and destination - should be expanded to current_dir?

Based on experience I have made after I created the PR I would prefer b), but maybe c) also makes sense.

@jarl-dk
Copy link
Member

jarl-dk commented Dec 10, 2014

Ok. I see. I tried to mimic the original programs.

Good idea. I consider it also good to mimic the interface of the methods in FileUtils

I think (c) due to API consistency.

@jarl-dk
Copy link
Member

jarl-dk commented Dec 10, 2014

Based on experience I have made after I created the PR I would prefer b), but maybe c) also makes sense.

I think this is probably because you have your source files in a fixture directory which is up higher than current_dir.

I think aruba could benefit to have a fixture directory configured and then maybe add support for fixture directory expansion like -/source_file.txt or ~~/source_file.txt (or some other syntax) to expand to a configured directory within the project directory. For that it should go for all the file access methods.

@ghost
Copy link
Author

ghost commented Dec 10, 2014

I think this is probably because you have your source files in a fixture directory which is up higher than current_dir.

Correct. :-)

I think aruba could benefit to have a fixture directory configured and then maybe add support for fixture directory expansion like -/source_file.txt or ~~/source_file.txt

Ok. I would vote for ~~/ or something like @-. This is more "visible" than just a single dash. Would you mind, if I introduce a new method expand_path or something like that? Otherwise the "new" expansion stuff would duplicated. If we chose @ as prefix for substitution variables it would be "re-usable" for other substitutions as well.

configured directory within the project directory

Should this be just an overwritable method?

@ghost
Copy link
Author

ghost commented Dec 10, 2014

TODO:

  • Move expand_paths-logic to single method

    Should be prefixed with current_dir if not prefixed with TBD

  • Define method for fixtures

  • Add logic to substitute TBD with value of fixtures directory

@jarl-dk
Copy link
Member

jarl-dk commented Dec 10, 2014

Yeah, that would be great.
I think the default value of this fixture direcory should do some searching (from project root, ie. execution dir) for presence of directory like spec/fixtures, features/fixtures, test/fixtures or something like that and of course it shall be configurable just like the timeout value.

Only one things to decide:

  1. the prefix for fixture directory: One or two characters? which characters?

If you create another PR for this, we can have a vote for the choice of the prefix? I can also imagine >/fixture_file.txt, ^/fixture_file.txt, |/fixture_file.txt, many options.

Maybe the whole concept of a fixture directory should be in cucumber project? Well, then again, not...

@ghost ghost mentioned this pull request Jan 15, 2015
6 tasks
@jarl-dk
Copy link
Member

jarl-dk commented Apr 4, 2015

Taking the fixture expansion feature out of this PR discussion. I think this feature (copy) should work just like the other file operation features, that is paths are relative to current directory with home directory expansion using ~.

@ghost
Copy link
Author

ghost commented Apr 7, 2015

@jarl-dk

I will rework this when the other PRs are merged/declined. Does this make sense?

@ghost ghost added the status: needs review label May 18, 2015
@ghost
Copy link
Author

ghost commented May 18, 2015

@jarl-dk @mattwynne

I refactored the PR and would like to ask you for feedback. Each commit changes separate bits of aruba.

@ghost ghost mentioned this pull request Jun 1, 2015
1 task
@ghost
Copy link
Author

ghost commented Jun 5, 2015

@mattwynne
Any chance that you review this today, as @jarl-dk is busy ? If you're ok with this PR, I'm happy to merge it myself. It becomes harder to keep track all opened PRs. 😄

source = source.flatten

source.each do |s|
raise ArgumentError, %(The following source "#{s}" does not exist.) unless exist? s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we extract this into #assert_exists?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmh... assert_* reads more like Test::Unit I prefer to leave it like it is. BTW: I'm trying to remove as much assert_-methods as possible (while keeping the api backwards-compatible for now). We may add a minitest-integration which brings them back. WDYT?

@maxmeyer
Copy link
Member

maxmeyer commented Jun 6, 2015

I rebased this PR.

@maxmeyer
Copy link
Member

maxmeyer commented Jun 9, 2015

Can we go on with that? 😄

@mattwynne
Copy link
Member

Go for it!

maxmeyer added a commit that referenced this pull request Jun 10, 2015
@maxmeyer maxmeyer merged commit 5bfd9d2 into cucumber:master Jun 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants