-
Notifications
You must be signed in to change notification settings - Fork 60
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
Added Padrino support by default #89
base: master
Are you sure you want to change the base?
Conversation
} | ||
|
||
options[:preload] = "config/boot.rb" if ENV['PADRINO_ENV'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that we need duplication here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I wanted to add the support out of box, instead of using --preload to point to "config/boot.rb" manually.
If you want to be more verbose, I can create a method which says "padrino_app?" then use this or use the Rails default path.
My only aim is to make is simple for people who are new to this, that's all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that we add config/boot.rb
to :preload
by default and then add it one more time for padrino.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I am sorry.
I meant the default to be "config/application.rb". Let me revert it back to orignal line.
Thanks! Could you also add a changelog entry and mention the Padrino support in README? |
I changed the README, changelog and the version. (did you want me to change the version?) Regards |
Thanks! Could you also revert the |
Done! |
@@ -23,6 +23,7 @@ module Spin | |||
class << self | |||
def serve(options) | |||
ENV['RAILS_ENV'] = 'test' unless ENV['RAILS_ENV'] | |||
ENV['PADRINO_ENV'] = 'test' unless ENV['PADRINO_ENV'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we set ENV['PADRINO_ENV']
here and then check if it's set in lib/spin/cli.rb
. Are you sure that's exactly what we need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am really sorry for these types of mistakes. We can't really use ENV to determine its a Padrino app (Like I originally thought). I didn't catch this bug because I was only testing it out on a Padrino app :/
Since Spin's philosophy is to run outside of your current app, I think its best to add as an option like (--padrino) and based on that I can add these configs.
Please share your thoughts on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also check if config/application.rb is there or not (Padrino doesn't have this file) and based on that determine if app is Padrino or Rails.
I don't think it's the best strategy to define "if there is no config/application.rb, then app is Padrino-based". |
I checked and I couldn't find any padrino specific file. For example there might be a mountable gem, which has dependency on Padrino and the core app is Rails or vice versa. (though if its a dependency then it won't appear in the Gemfile, it will be in the Gemfile.lock) I am leaning more towards just giving an option to the user (--padrino), so we don't have to break our heads to determine if its Padrino app or not. |
Maybe you can use |
Hey,
I just added a ENV variable and a default preload path to make it work with Padrino out of the box.
Thank you making this gem, It was so simple to port it :)