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

Added Padrino support by default #89

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

Who828
Copy link

@Who828 Who828 commented Nov 17, 2013

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 :)

}

options[:preload] = "config/boot.rb" if ENV['PADRINO_ENV']
Copy link
Collaborator

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?

Copy link
Author

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.

Copy link
Collaborator

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.

Copy link
Author

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.

@kirs
Copy link
Collaborator

kirs commented Nov 17, 2013

Thanks! Could you also add a changelog entry and mention the Padrino support in README?

@Who828
Copy link
Author

Who828 commented Nov 17, 2013

I changed the README, changelog and the version. (did you want me to change the version?)
Let me know if there anything more to be done.

Regards

@kirs
Copy link
Collaborator

kirs commented Nov 17, 2013

Thanks! Could you also revert the lib/spin/version.rb?

@Who828
Copy link
Author

Who828 commented Nov 17, 2013

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']
Copy link
Collaborator

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?

Copy link
Author

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

Copy link
Author

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.

@kirs
Copy link
Collaborator

kirs commented Nov 17, 2013

I don't think it's the best strategy to define "if there is no config/application.rb, then app is Padrino-based".
Is there any Padrino-specific file?

@Who828
Copy link
Author

Who828 commented Nov 17, 2013

I checked and I couldn't find any padrino specific file.
We can also check the Gemfile (see if it has Padrino or not) but that might increase the complexity and it might not always be correct.

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.

@parasnarang
Copy link

Maybe you can use if defined?(Padrino)

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