Add a logging layer to address common issues #381

Merged
merged 2 commits into from Nov 2, 2016

Conversation

Projects
None yet
2 participants
@michaelherold
Contributor

michaelherold commented Nov 1, 2016

One of the most common issues we have reported is when a Hashie::Mash
attempts to override a built-in method and fails to do so. To prevent
this from being an issue in the future, I added a logging layer
accessible at Hashie.logger that is user-configurable. This should
help to report the error to the user before it becomes a problem.

The logging layer is a barebones implementation right now. It defaults
to a standard Logger.new($stdout) but can be overridden by setting
Hashie.logger = MyAwesomeLogger.new. Ideally, we can expand this layer
and its usage to make the user experience better for Hashie.

Closes #361

spec/hashie/mash_spec.rb
+ it 'logs a warning when overriding built-in methods' do
+ original_logger = Hashie.logger
+ stdout = StringIO.new
+ Hashie.logger = Logger.new(stdout)

This comment has been minimized.

@dblock

dblock Nov 1, 2016

Contributor

Should be around the test, before and after, maybe with context, otherwise on failure it doesn't reset.

@dblock

dblock Nov 1, 2016

Contributor

Should be around the test, before and after, maybe with context, otherwise on failure it doesn't reset.

This comment has been minimized.

@michaelherold

michaelherold Nov 1, 2016

Contributor

Good call! Thanks for catching that.

@michaelherold

michaelherold Nov 1, 2016

Contributor

Good call! Thanks for catching that.

spec/hashie_spec.rb
+RSpec.describe Hashie do
+ describe '.logger' do
+ it 'is available via an accessor' do
+ stdout = StringIO.new

This comment has been minimized.

@dblock

dblock Nov 1, 2016

Contributor

Same as above.

I would create a shared context with logger.

@dblock

dblock Nov 1, 2016

Contributor

Same as above.

I would create a shared context with logger.

README.md
@@ -28,6 +28,15 @@ The library is broken up into a number of atomically includable Hash extension m
Any of the extensions listed below can be mixed into a class by `include`-ing `Hashie::Extensions::ExtensionName`.
+## Logging
+
+Hashie has a built-in logger that you can override. By default, it logs to `$stdout` but can be replaced by any `Logger` class. The logger is accessible on the Hashie module, as shown below:

This comment has been minimized.

@dblock

dblock Nov 1, 2016

Contributor

I always get confused about what's better, $stdout or STDOUT? No need to change anything, just see both versions around. Of course $STDOUT doesn't exist.

2.2.1 :001 > $stdout
 => #<IO:<STDOUT>> 
2.2.1 :002 > $STDOUT
 => nil 
2.2.1 :003 > STDOUT
 => #<IO:<STDOUT>> 
2.2.1 :004 > 
@dblock

dblock Nov 1, 2016

Contributor

I always get confused about what's better, $stdout or STDOUT? No need to change anything, just see both versions around. Of course $STDOUT doesn't exist.

2.2.1 :001 > $stdout
 => #<IO:<STDOUT>> 
2.2.1 :002 > $STDOUT
 => nil 
2.2.1 :003 > STDOUT
 => #<IO:<STDOUT>> 
2.2.1 :004 > 

This comment has been minimized.

@michaelherold

michaelherold Nov 1, 2016

Contributor

I flip flop on this as well, though I think I prefer STDOUT now that you mention it. I think I'll update the PR to use that.

@michaelherold

michaelherold Nov 1, 2016

Contributor

I flip flop on this as well, though I think I prefer STDOUT now that you mention it. I think I'll update the PR to use that.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Nov 1, 2016

Contributor

I like it. Just make those tests a bit more consistent/prettier and it's good to go.

Contributor

dblock commented Nov 1, 2016

I like it. Just make those tests a bit more consistent/prettier and it's good to go.

michaelherold added some commits Nov 1, 2016

Add a logging layer to address common issues
One of the most common issues we have reported is when a `Hashie::Mash`
attempts to override a built-in method and fails to do so. To prevent
this from being an issue in the future, I added a logging layer
accessible at `Hashie.logger` that is user-configurable. This should
help to report the error to the user before it becomes a problem.

The logging layer is a barebones implementation right now. It defaults
to a standard `Logger.new($stdout)` but can be overridden by setting
`Hashie.logger = MyAwesomeLogger.new`. Ideally, we can expand this layer
and its usage to make the user experience better for Hashie.

Closes #361
@michaelherold

This comment has been minimized.

Show comment
Hide comment
@michaelherold

michaelherold Nov 1, 2016

Contributor

Thanks for the feedback @dblock! I changed around the specs to match your suggestions and they look a lot neater - I hadn't thought to eliminate setup code like that before.

Can I get a sign-off or second round of review, please?

Contributor

michaelherold commented Nov 1, 2016

Thanks for the feedback @dblock! I changed around the specs to match your suggestions and they look a lot neater - I hadn't thought to eliminate setup code like that before.

Can I get a sign-off or second round of review, please?

@@ -0,0 +1,25 @@
+require 'spec_helper'
+
+def a_method_to_match_against

This comment has been minimized.

@dblock

dblock Nov 2, 2016

Contributor

Generally I stay away from global methods, I would move it inside RSpec.describe which I think namespaces it, or namespace it explicitly. It's ok for now.

@dblock

dblock Nov 2, 2016

Contributor

Generally I stay away from global methods, I would move it inside RSpec.describe which I think namespaces it, or namespace it explicitly. It's ok for now.

@dblock dblock merged commit e35e628 into intridea:master Nov 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Nov 2, 2016

Contributor

Merged, thanks.

Contributor

dblock commented Nov 2, 2016

Merged, thanks.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Jan 31, 2017

Contributor

What was the reason not to auto-load logger here? See https://github.com/intridea/hashie/pull/392/files in which I had to move it.

Contributor

dblock commented Jan 31, 2017

What was the reason not to auto-load logger here? See https://github.com/intridea/hashie/pull/392/files in which I had to move it.

@michaelherold michaelherold deleted the michaelherold:features/logging-layer branch Jan 31, 2017

@michaelherold

This comment has been minimized.

Show comment
Hide comment
@michaelherold

michaelherold Jan 31, 2017

Contributor

I wanted it to be globally accessible. Moving it is fine with me.

Autoloading breaks my brain.

Contributor

michaelherold commented Jan 31, 2017

I wanted it to be globally accessible. Moving it is fine with me.

Autoloading breaks my brain.

@bmclean bmclean referenced this pull request in omniauth/omniauth Jan 31, 2017

Closed

Hashie warnings #872

@shortdudey123 shortdudey123 referenced this pull request in dorack/jiralicious Mar 7, 2017

Open

Getting a conflict on Hashie::Mash #46

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Mar 20, 2017

taca
Update ruby-hashie to 3.5.5.
# Change Log

## [3.5.5] - 2017-02-24

[3.5.5]: intridea/hashie@v3.5.4...v3.5.5

### Added

* [#326](intridea/hashie#326): Added `Hashie::Extensions::Mash::KeepOriginalKeys` to give Mashes the ability to keep the original structure given to it - [@michaelherold](https://github.com/michaelherold).

### Fixed

* [#415](intridea/hashie#415): Fixed Mash logging keys multiple times which lead to a bad user experience or, in some cases, errors - [@michaelherold](https://github.com/michaelherold).

## [3.5.4] - 2017-02-22

[3.5.4]: intridea/hashie@v3.5.3...v3.5.4

### Added

* [#412](intridea/hashie#412): Added a Hashie::Extensions::Mash::SymbolizeKeys extension that overrides the default stringification behavior for keys - [@michaelherold](https://github.com/michaelherold).

### Fixed

* [#409](intridea/hashie#409): Fixed Railtie detection for projects where Rails is defined but Railties are not availble - [@CallumD](https://github.com/callumd).
* [#411](intridea/hashie#411): Fixed a performance regression from 3.4.3 that caused a 10x slowdown in OmniAuth - [@michaelherold](https://github.com/michaelherold).

## [3.5.3] - 2017-02-11

[3.5.3]: intridea/hashie@v3.5.2...v3.5.3

### Fixed

* [#402](intridea/hashie#402): Use a Railtie to set Hashie.logger on rails boot - [@matthewrudy](https://github.com/matthewrudy).
* [#406](intridea/hashie#406): Ensure that subclasses that disable warnings propagate that setting to grandchild classes - [@michaelherold](https://github.com/michaelherold).
* Your contribution here.

## [3.5.2] - 2017-02-10

[3.5.2]: intridea/hashie@v3.5.1...v3.5.2

### Added

* [#395](intridea/hashie#395): Add the ability to disable warnings in Mash subclasses - [@michaelherold](https://github.com/michaelherold).
* [#400](intridea/hashie#400): Fix Hashie.logger load and set the Hashie logger to the Rails logger in a Rails environment - [@michaelherold](https://github.com/michaelherold).

### Fixed

* [#396](intridea/hashie#396): Fix for specs in #381: Incorrect use of shared context meant example was not being run - [@biinari](https://github.com/biinari).
* [#399](intridea/hashie#399): Fix passing Pathname object to Hashie::Mesh.load() - [@albb0920](https://github.com/albb0920).

### Miscellanous

* [#397](intridea/hashie#397): Add the integration specs harness into the main test tasks - [@michaelherold](https://github.com/michaelherold).

## [3.5.1] - 2017-01-31

* [#392](intridea/hashie#392): Fix for #391: Require all dependencies of Hashie::Mash - [@dblock](https://github.com/dblock).

[3.5.1]: intridea/hashie@v3.5.0...v3.5.1

## [3.5.0] - 2017-01-31

* [#386](intridea/hashie#386): Fix for #385: Make `deep_merge` always `deep_dup` nested hashes before merging them in so that there are no shared references between the two hashes being merged. - [@mltsy](https://github.com/mltsy).
* [#389](intridea/hashie#389): Support Ruby 2.4.0 - [@camelmasa](https://github.com/camelmasa).

[3.5.0]: intridea/hashie@v3.4.6...v3.5.0

### Added

* [#381](intridea/hashie#381): Add a logging layer that lets us report potential issues to our users. As the first logged issue, report when a `Hashie::Mash` is attempting to overwrite a built-in method, since that is one of our number one questions - [@michaelherold](https://github.com/michaelherold).

### Changed

* [#384](intridea/hashie#384): Updated to CodeClimate 1.x - [@boffbowsh](https://github.com/boffbowsh).

### Fixed

* [#369](intridea/hashie#369): If a translation for a property exists when using IndifferentAccess and IgnoreUndeclared, use the translation to find the property - [@whitethunder](https://github.com/whitethunder).
* [#376](intridea/hashie#376): Leave string index unchanged if it can't be converted to integer for Array#dig - [@sazor](https://github.com/sazor).
* [#377](intridea/hashie#377): Dont use Rubygems to check ruby version - [@sazor](https://github.com/sazor).
* [#378](intridea/hashie#378): Deep find all searches inside all nested hashes - [@sazor](https://github.com/sazor).
* [#380](intridea/hashie#380): Evaluate procs default values of Dash in object initialization - [@sazor](https://github.com/sazor).

### Miscellanous

* [#387](intridea/hashie#387): Fix builds failing due to Rake 11 having a breaking change - [@michaelherold](https://github.com/michaelherold).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment