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

Refactored CLI override command to use Thor. #2042

Merged
merged 1 commit into from
Dec 5, 2012
Merged

Conversation

parndt
Copy link
Member

@parndt parndt commented Nov 25, 2012

WARNING: The tests FAIL

This is because cli_spec.rb is testing the output of the rake command. I think this is silly.

If we're testing the CLI we should be testing the CLI class, not the rake command.

Permission to refactor these specs to test the class instead?

Here's how you'd call it:

Refinery::CLI.new.override({"view" => "_content_page"})

Seems better to me.

@robyurkowski @ugisozols Please give me your feedback.

@robyurkowski
Copy link
Contributor

I think this is absolutely the right move. That also gives us the ability for end users to write custom scripts, which is pretty sweet too...

@parndt
Copy link
Member Author

parndt commented Nov 25, 2012

Not only that but we can remove all the ugly FileUtils.rm_r stuff with this:

Refinery::CLI.new([], {:pretend => true}).override({"view" => "_content_page"})

Note how this works:

[1] pry(main)> Refinery::CLI.new([], {:pretend => true}).override({"view" => "_content_page"})
      create  spec/dummy/app/views/refinery/_content_page.html.erb
=> ["refinery/_content_page.html.erb"]
[2] pry(main)> Refinery::CLI.new([], {:pretend => true}).override({"view" => "_content_page"})
      create  spec/dummy/app/views/refinery/_content_page.html.erb
=> ["refinery/_content_page.html.erb"]
[3] pry(main)> Refinery::CLI.new([], {:pretend => false}).override({"view" => "_content_page"})
      create  spec/dummy/app/views/refinery/_content_page.html.erb
=> ["refinery/_content_page.html.erb"]
[4] pry(main)> Refinery::CLI.new([], {:pretend => true}).override({"view" => "_content_page"})
   identical  spec/dummy/app/views/refinery/_content_page.html.erb
=> ["refinery/_content_page.html.erb"]

File not created unless pretend is false and it shows us identical output to the true copy. Movie magic.

@robyurkowski
Copy link
Contributor

I think it'll also lend itself to be moved to the refinerycms binary, with an API that looks something like refinerycms override view _content_page.

@parndt
Copy link
Member Author

parndt commented Nov 25, 2012

Except it needs the environment so that it has Refinery::Plugins.registered so it might as well be a rake task?

@ugisozols
Copy link
Member

Looks good and much cleaner than current implementation. Permission granted to change specs ;)

@parndt
Copy link
Member Author

parndt commented Dec 5, 2012

Specs updated :) much cleaner!

ugisozols added a commit that referenced this pull request Dec 5, 2012
Refactored CLI override command to use Thor.
@ugisozols ugisozols merged commit 14be24f into master Dec 5, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants