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

Introduce ApiTaster.route_path configuration option #45

Merged
merged 1 commit into from
Jul 19, 2013

Conversation

timurvafin
Copy link
Contributor

@fredwu,

The right way to define where we would like to use ApiTaster is to require it in the correct bundler group.
For example we usually would like to use it in the development and staging environments in the Rails application.

So make sense to require like:

group :development, :staging do
  gem 'api_taster', github: 'fredwu/api_taster'
end

In the config/routes.rb better to check ApiTaster constant definition instead of define tricky logic like this

if Rails.env.development? || Rails.env.staging?
  mount ApiTaster::Engine => '/api_taster'
end

By default Rails loader requires everything from app directory. So in case we are in the test environment for example when api_taster gem does not loaded we will get uninitialized constant ApiTaster error for sure.

And for sure it will be not cool to check ApiTaster constant definition in each file in the app/api_tasters/ folder.
Probably better to move api tasters from app/api_tasters to lib/api_tasters.

So this change set implements two things:

  1. ApiTaster.route_path option introduced instead of hardcoded value in the Route class
  2. Default value changed to lib/api_tasters
  • Timur

@timurvafin
Copy link
Contributor Author

@fredwu it will be great to get some feedback )

@fredwu
Copy link
Owner

fredwu commented Jul 18, 2013

Hi @timurvafin thank you so much for the PR with detailed explanation! 👍 Sorry I haven't been responsive on this project lately, I will go over it in the next few days.

Is this the same issue as #43?

@timurvafin
Copy link
Contributor Author

@fredwu right #43 is the same issue and that PR could be ignored.

fredwu added a commit that referenced this pull request Jul 19, 2013
Introduce ApiTaster.route_path configuration option
@fredwu fredwu merged commit 8ac3021 into fredwu:master Jul 19, 2013
@RobWu RobWu mentioned this pull request Mar 11, 2014
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.

2 participants