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

Engine requires bourbon, fixes #615 #673

Closed
wants to merge 12 commits into from

Conversation

kpheasey
Copy link
Contributor

No description provided.

@@ -14,3 +14,4 @@
/tags
/tmp/*
/spec/example_app/tmp/*
.idea
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking this should probably be in your personal global .gitignore rather in Administrate's .gitignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never knew there was global .gitignore, thanks for the knowledge!

I'll revert it

@JoelQ
Copy link
Contributor

JoelQ commented Oct 28, 2016

I'd love to get some testing around this. Currently the test suite is green (if you rebase the latest master) without this change. Can we make the sample_app fail the same way as everyone else and then this fix makes it go green?

@kpheasey
Copy link
Contributor Author

Rebased with master and removed the .gitignore stuff.

Not sure how to test the require statement in the sample app. Since the sample app uses the engine's gemfile bourbon gets included. When bourbon is not in the gemfile, it is not required by the engine.

@JoelQ
Copy link
Contributor

JoelQ commented Oct 28, 2016

Ah, I see what you're saying.

I think we can add require: false to bourbon in the Gemfile. This will prevent it from being auto-required. Doing this on my machine caused all the feature specs to fail with:

ActionView::Template::Error:
        File to import not found or unreadable: bourbon.

Adding in your require to engine.rb then caused the test suite to go green.

Alternatively, we could try to go down the path of having a separate Gemfile for the sample app.

Thoughts?

cc @danbee @BenMorganIO

@kpheasey
Copy link
Contributor Author

I believe dependencies should be defined in the gemspec, http://guides.rubyonrails.org/engines.html#other-gem-dependencies http://guides.rubyonrails.org/engines.html#other-gem-dependencies

If you define a dependency in the gemspec, it will need to be explicitly required upon initialization. This would avoid further confusion and not require testing.

On Oct 28, 2016, at 3:35 PM, Joël Quenneville notifications@github.com wrote:

Ah, I see what you're saying.

I think we can add require: false to bourbon in the Gemfile. This will prevent it from being auto-required. Doing this on my machine caused all the feature specs to fail with:

ActionView::Template::Error:
File to import not found or unreadable: bourbon.
Adding in your require to engine.rb then caused the test suite to go green.

Alternatively, we could try to go down the path of having a separate Gemfile for the sample app.

Thoughts?

@BenMorganIO
Copy link
Collaborator

@kpheasey could you squash your three commits? I think they could all be just one in this instance.

@JoelQ
Copy link
Contributor

JoelQ commented Oct 28, 2016

I believe dependencies should be defined in the gemspec

Good point. It looks like we have some dependencies just in the Gemfile, some just in the gemspec, and some in both. Not sure why that happened.

Do you want turn this PR into a migration from using the Gemfile to using the gemspec? Doing that should both fix the bourbon problem and fail the specs when not requiring he proper files.

Kevin Pheasey and others added 9 commits October 28, 2016 16:50
Per [issue thoughtbot#647], documentation does not contain possible options to set
for fields via .with_options.

* Add possible options for Field::HasMany, Field::Number, Field::Select,
Field::String, and Field::Text
* Remove :multiplier and :title options from Field::Number.with_options
example, as they are not yet implemented (see [issue thoughtbot#292])

[issue thoughtbot#647]: thoughtbot#647
[issue thoughtbot#292]: thoughtbot#292

ignore rubymine files

administrate engine requires bourbon. fixes thoughtbot#615

Revert "ignore rubymine files"

This reverts commit 4d82e06.

administrate engine requires bourbon. fixes thoughtbot#615

ignore rubymine files
@kpheasey
Copy link
Contributor Author

I've completely screwed this up trying to squash, I'm going to open a new pull request with a separate branch, sorry about that.

@JoelQ
Copy link
Contributor

JoelQ commented Oct 28, 2016

No worries @kpheasey 😃

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.

4 participants