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

Bug undefined blank for string in Rollbar::Logger #675

Merged

Conversation

duksis
Copy link
Contributor

@duksis duksis commented Feb 1, 2018

Possible solution to close #674 - feel free to use it or not to :)

Implements a local blank for usage without core_ext
Check if message responds to :blank?, if it does not
use a local implementation of it.

local implementation taken from
https://github.com/rails/rails/blob/v5.1.4/activesupport/lib/active_support/core_ext/object/blank.rb#L17

Rollbar does not have ActiveSupport as
a runtime dependency in gemspec and it is
not required in Rollbar::Logger but is used.

way to reproduce:

```irb
require 'rollbar/logger'
Rollbar::Logger.new.error("foo")
  # NoMethodError: undefined method `blank?' for "foo":String
```

can be worked arround by requiring
`active_support/core_ext/object/blank`

```irb
require 'rollbar/logger'
require 'active_support/core_ext/object/blank'
Rollbar::Logger.new.error("foo")
  # => "disabled"
```
Check if message responds to :blank?, if it does not
use a local implementation of it.

local implementation taken from
https://github.com/rails/rails/blob/v5.1.4/activesupport/lib/active_support/core_ext/object/blank.rb#L17
@ArturMoczulski
Copy link
Contributor

@duksis why not just add a dependency on active_support/core_ext in the Gemfile rather than duplicating their implementation?

@duksis
Copy link
Contributor Author

duksis commented Feb 7, 2018

@ArturMoczulski it's always good to keep your dependencies as minimal as possible.
At the moment rollbar-gem has only one dependency in gemspec and in order to reuse active_support/core_ext instead of copying this one line it would be needed to add the whole activesupport as a runtime dependency.

at the end it's a decision that the maintainers have to make:

4 lines with a guard clause and 1 line copied from core ext
VS
371 KB of activesupport code in dependencies

@ArturMoczulski
Copy link
Contributor

@duksis that's a good point

@ArturMoczulski ArturMoczulski merged commit eba9ba7 into rollbar:master Feb 8, 2018
@duksis duksis deleted the bug-undefined-blank-for-string branch February 8, 2018 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rollbar::Logger uses active_support/core_ext without depending on it
3 participants