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

Delayed log overwrites logger config? #353

Closed
strathmeyer opened this issue Jan 3, 2013 · 9 comments
Closed

Delayed log overwrites logger config? #353

strathmeyer opened this issue Jan 3, 2013 · 9 comments
Milestone

Comments

@strathmeyer
Copy link
Contributor

I can't seem to set a custom logger via Savon.client(logger: my_logger)

If I read the code correctly, the log option is delayed until after the logger option is set, but the setting of the log option overwrites the provided logger and sets it to be either a $stdout logger, or a /dev/null logger:
https://github.com/savonrb/savon/blob/master/lib/savon/options.rb#L137-148

I'll come up with a spec.

@rubiii
Copy link
Contributor

rubiii commented Jan 3, 2013

thanks for reporting this. i'll wait for your spec then.

@strathmeyer
Copy link
Contributor Author

Added failing spec.

@strathmeyer
Copy link
Contributor Author

Added code to fix failing spec.

(It's still non-intuitive that if you use log:false, it clobbers the custom logger and uses a /dev/null logger, instead of actually turn off logging.)

@rubiii
Copy link
Contributor

rubiii commented Jan 3, 2013

thanks @strathmeyer. unfortunately, i have several other tickets to solve before this one, but i really
appreciate your help!

savon 1.2 featured a NullLogger which replaced the default logger when someone turns off logging:
https://github.com/savonrb/savon/blob/v1.2.0/lib/savon/config.rb#L27

it's a simple null object which just silently ignores log messages. a better concept than littering the
code which if statements imho. i changed that to have the default logger log to /dev/null in 2.0,
because it required less code and seemed to be simpler to understand.

i'm not sure which option is better right now. maybe if you could explain what you find "non-intuitiv"
about the current solution and how you like the null object approach vs. conditionals, we can find
a good solution and move this ticket forward a little bit faster.

let me know what you think.

@strathmeyer
Copy link
Contributor Author

What is non-intuitive is that you can set the logger, but if you give log: false, then the logger on the resulting client is neither nil, nor the logger you passed. I guess it's not that bad.

How do you feel about the pull request as it currently stands?

@rubiii
Copy link
Contributor

rubiii commented Jan 4, 2013

hey @strathmeyer. i quickly had a look at your pull request and added a few comments.

@rubiii
Copy link
Contributor

rubiii commented Jan 4, 2013

i thought about your comment that this is non-intuitive and i agree.

maybe it's easier to simply solve this by storing the information whether we should log or not
and to wrap the actual log request/response log methods with simple if statements.

not a great design decision, but at least it doesn't hide any weird implementation details.

@rubiii
Copy link
Contributor

rubiii commented Jan 6, 2013

cherry-picked your spec and simplified the implementation.
could you please let me know whether this works for you?

@rubiii
Copy link
Contributor

rubiii commented Feb 3, 2013

released v2.1.0. please refer to the updated changelog and documentation for details. let me know how it works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants