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

Feature/engine paths #114

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@KensoDev

KensoDev commented Jul 4, 2012

Fabrication was not working with Engines, the fabricators folder was expected to be inside the dummy app
this pull request fixes it.

  • Refactored the code to extract Rails constant, so I can stub it in the specs
  • Added specs to the engine path.
  • All suite passes, resynced with master right before I pushed this one.
@paulelliott

This comment has been minimized.

Show comment
Hide comment
@paulelliott

paulelliott Jul 6, 2012

Owner

I don't think this is an appropriate thing for Fabrication to handle. If you need to load fabricators from an engine or anywhere else, you can add to the fabricator search path.

http://fabricationgem.org/#!configuration

Owner

paulelliott commented Jul 6, 2012

I don't think this is an appropriate thing for Fabrication to handle. If you need to load fabricators from an engine or anywhere else, you can add to the fabricator search path.

http://fabricationgem.org/#!configuration

@paulelliott paulelliott closed this Jul 6, 2012

@KensoDev

This comment has been minimized.

Show comment
Hide comment
@KensoDev

KensoDev Jul 6, 2012

@paulelliott When I tried it, this didn't work and it seemed that the path is always relative to the Rails.root.
When you are working on an engine, the path is outside of the rails root, since the app sits inside spec/dummy.

Nothing I did seemed to work, that is why I decided to have the core of the library handles it.

Bottom line
The configuration is not working for me if I want to configure the engine outside of the app (like in a rails engine)

KensoDev commented Jul 6, 2012

@paulelliott When I tried it, this didn't work and it seemed that the path is always relative to the Rails.root.
When you are working on an engine, the path is outside of the rails root, since the app sits inside spec/dummy.

Nothing I did seemed to work, that is why I decided to have the core of the library handles it.

Bottom line
The configuration is not working for me if I want to configure the engine outside of the app (like in a rails engine)

@paulelliott

This comment has been minimized.

Show comment
Hide comment
@paulelliott

paulelliott Jul 6, 2012

Owner

What is the path of the engine and of your Rails app?

Owner

paulelliott commented Jul 6, 2012

What is the path of the engine and of your Rails app?

@KensoDev

This comment has been minimized.

Show comment
Hide comment
@KensoDev

KensoDev Jul 6, 2012

Engine
/Users/avitzurel/projects/graph_engine

App
/Users/avitzurel/projects/graph_engine/spec/dummy

KensoDev commented Jul 6, 2012

Engine
/Users/avitzurel/projects/graph_engine

App
/Users/avitzurel/projects/graph_engine/spec/dummy

@paulelliott

This comment has been minimized.

Show comment
Hide comment
@paulelliott

paulelliott Jul 7, 2012

Owner

Can you try adding this under spec/support/fabrication.rb in the Rails app? Unless I am missing something, this should work.

Fabrication.configure do |config|
  config.fabricator_path.push('spec/dummy/spec/fabricators')
end
Owner

paulelliott commented Jul 7, 2012

Can you try adding this under spec/support/fabrication.rb in the Rails app? Unless I am missing something, this should work.

Fabrication.configure do |config|
  config.fabricator_path.push('spec/dummy/spec/fabricators')
end
@KensoDev

This comment has been minimized.

Show comment
Hide comment
@KensoDev

KensoDev Jul 7, 2012

I don't want the fabricators inside the spec app inside the dummy app.
I want the fabricators in the folder where my engine lives.

If I wanted it inside the app, this is already working perfectly.

This is the folder structure I want

The fabricators are in the spec directory for the engine, outside the dummy app.

KensoDev commented Jul 7, 2012

I don't want the fabricators inside the spec app inside the dummy app.
I want the fabricators in the folder where my engine lives.

If I wanted it inside the app, this is already working perfectly.

This is the folder structure I want

The fabricators are in the spec directory for the engine, outside the dummy app.

@KensoDev

This comment has been minimized.

Show comment
Hide comment
@KensoDev

KensoDev commented Jul 11, 2012

@paulelliott News on this?

@paulelliott

This comment has been minimized.

Show comment
Hide comment
@paulelliott

paulelliott Jul 12, 2012

Owner

The code in this pull request uses paths that are hard-coded for your application. I am not totally opposed to adding in some sort of support for engines, but if we do then it needs to be more flexible. We may be able to include engine_paths as a configuration option instead and run the code based on the values presented there. Requiring them to be supplied like that would mean that we don't need to do any lookups at all.

Owner

paulelliott commented Jul 12, 2012

The code in this pull request uses paths that are hard-coded for your application. I am not totally opposed to adding in some sort of support for engines, but if we do then it needs to be more flexible. We may be able to include engine_paths as a configuration option instead and run the code based on the values presented there. Requiring them to be supplied like that would mean that we don't need to do any lookups at all.

@paulelliott paulelliott reopened this Jul 12, 2012

@michaelrigart

This comment has been minimized.

Show comment
Hide comment
@michaelrigart

michaelrigart Jul 12, 2012

Found myself in the same situation where Fabrication indeed looks inside the test folder of the dummy app.
I have to agree with KensoDev that this isn't the right place to store them. But at the same time i agree with paulelliott that the code in the pull request looks a bit hackery :)

can't we just make the path_prefix configurable (not obligatory)? This way, someone that uses engines, can set the path_prefix to MyEngine::Engine.root .

If path_prefix has not been configured, use the existing logic ( if Rails defined, use Rails.root, else ....)

michaelrigart commented Jul 12, 2012

Found myself in the same situation where Fabrication indeed looks inside the test folder of the dummy app.
I have to agree with KensoDev that this isn't the right place to store them. But at the same time i agree with paulelliott that the code in the pull request looks a bit hackery :)

can't we just make the path_prefix configurable (not obligatory)? This way, someone that uses engines, can set the path_prefix to MyEngine::Engine.root .

If path_prefix has not been configured, use the existing logic ( if Rails defined, use Rails.root, else ....)

@KensoDev

This comment has been minimized.

Show comment
Hide comment
@KensoDev

KensoDev Jul 16, 2012

I agree that this seems hackery.

IMHO the gem still needs to add support for that, because in the current situation the gem is not usable when you are working on an Engine.

So... What are the suggestions? What do you think is a good solution?
I would be happy to implement

KensoDev commented Jul 16, 2012

I agree that this seems hackery.

IMHO the gem still needs to add support for that, because in the current situation the gem is not usable when you are working on an Engine.

So... What are the suggestions? What do you think is a good solution?
I would be happy to implement

@michaelrigart

This comment has been minimized.

Show comment
Hide comment
@michaelrigart

michaelrigart Jul 17, 2012

I have opened a pull request #116 for a small patch (#116) that makes the path_prefix configurable.

Using this patch at the moment and it's working for me.

michaelrigart commented Jul 17, 2012

I have opened a pull request #116 for a small patch (#116) that makes the path_prefix configurable.

Using this patch at the moment and it's working for me.

@paulelliott

This comment has been minimized.

Show comment
Hide comment
@paulelliott

paulelliott Jul 18, 2012

Owner

Version 2.2.0 has pull request #116 merged in so you can now specify the path prefix for your engine.

http://www.fabricationgem.org/#!configuration

Owner

paulelliott commented Jul 18, 2012

Version 2.2.0 has pull request #116 merged in so you can now specify the path prefix for your engine.

http://www.fabricationgem.org/#!configuration

@KensoDev

This comment has been minimized.

Show comment
Hide comment
@KensoDev

KensoDev Jul 18, 2012

That is awesome, thank you.

KensoDev commented Jul 18, 2012

That is awesome, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment