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

If TZInfo-data is not present in windows, let the user know. #19180

Merged
merged 1 commit into from Mar 5, 2015

Conversation

sivsushruth
Copy link
Contributor

Fixes #19175.
Rails tries calling TZInfo::DataSource.get. If it gets a DataSourceNotFound error then it prints out

Gem 'tzinfo-data' is not present. Please add gem 'tzinfo-data' to your Gemfile and run bundle install

begin
TZInfo::DataSource.get
rescue DataSourceNotFound
Logger = ActiveSupport::Logger.new(STDOUT)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use the Rails.logger here. Or even throw a warning instead.

Copy link
Member

Choose a reason for hiding this comment

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

We should just raise. We don't want the boot to continue.

@sivsushruth
Copy link
Contributor Author

If tzinfo-data not found, it aborts with the message "tzinfo-data' is not present. Please add gem 'tzinfo-data' to your Gemfile and run bundle install"

rescue TZInfo::DataSourceNotFound
abort <<-end_message

tzinfo-data' is not present. Please add gem 'tzinfo-data' to your Gemfile and run bundle install
Copy link
Member

Choose a reason for hiding this comment

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

why the extra ' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo, fixing it.
Thanks.

@arthurnn
Copy link
Member

arthurnn commented Mar 3, 2015

I also wonder if we can test this somehow..
Also if you could squash all commits in one, so we have a smaller history.
thanks

@sivsushruth
Copy link
Contributor Author

Re-raise the exception with a message.

rescue TZInfo::DataSourceNotFound => e
  raise e.exception "tzinfo-data is not present. Please add gem 'tzinfo-data' to your Gemfile and run bundle install"
end

@sivsushruth
Copy link
Contributor Author

@arthurnn @rafaelfranca I chose to raise an exception instead of aborting directly because it would allow for users to rescue the error if they wanted.
Let me know what you think.

Thanks

@arthurnn
Copy link
Member

arthurnn commented Mar 4, 2015

@sivsushruth I like raising an error 👍 , I still think we should add a regression tests somewhere to make sure this works, and to make sure nobody removes this code.

@sivsushruth
Copy link
Contributor Author

@arthurnn @rafaelfranca I cant seem to figure out how/where to write tests for initializer . Any suggestions?

Thanks

@arthurnn
Copy link
Member

arthurnn commented Mar 5, 2015

I will merge this for now. We dont have windows only tests as far as I can tell, so it makes kinda harder to test this.
If anyone has any suggestion on how to add test to this, please ping me in the PR and add a reference to this.
thanks all

arthurnn pushed a commit that referenced this pull request Mar 5, 2015
If TZInfo-data is not present in windows, let the user know.
@arthurnn arthurnn merged commit 3c94bdd into rails:master Mar 5, 2015
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.

tzinfo-data warning on boot if not present in Gemfile
4 participants