Fix config.log_silently? #324
Conversation
|
|
||
| it "log_silently? should always be true if ENV['RAILS_GROUPS'] == 'assets'" do | ||
| AssetSync.config.log_silently = false | ||
| expect(ENV).to receive(:[]).with('RAILS_GROUPS').and_return('assets') |
There was a problem hiding this comment.
I think it should be allow(ENV)?
Since It looks like part of test setup to me.
If I guess it wrong please tell me what it is.
There was a problem hiding this comment.
expect(ENV) has the same behavior (stubbing) as allow(ENV) but also sets up an expectation that the var will be checked in ENV. Just adds more coverage, but I can change it if you would prefer.
There was a problem hiding this comment.
OK I think it's fine to have expect
But you should add a comment explaining what that line does (stub + expect)
It's easier for people to read comments than finding history => PR => Line note :)
Edit: Changed some wording
There was a problem hiding this comment.
I think it is self-explanatory, but I have added a comment to be sure :)
| @@ -94,7 +94,7 @@ def fail_silently? | |||
| end | |||
|
|
|||
| def log_silently? | |||
| ENV['RAILS_GROUPS'] == 'assets' || self.log_silently == false | |||
There was a problem hiding this comment.
Should we consider nil as false?
I know the default value is true, but if someone accidentally assigned nil to that attribute
Should we throw warning or something?
There was a problem hiding this comment.
Maybe if we warned on nil elsewhere in handling the config options? For something as benign as logging, casting nil to false is probably OK :)
There was a problem hiding this comment.
OK let's do it later (maybe until someone complains)
|
Won't wait, he is too slow :( |
log_silently?would almost always return true, now it honors the config value unless theRAILS_GROUPSenv var is set toassets.