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

Set HTTPI logger to Savon logger #636

Merged
merged 2 commits into from
Nov 12, 2014
Merged

Conversation

krissi
Copy link
Contributor

@krissi krissi commented Oct 24, 2014

This PR sets the HTTPI logger to the Savon logger in addition to the log flag.

@tjarratt
Copy link
Contributor

Thanks for the pull request @krissi!

I'm not convinced that the specs for this change are valuable, because it doesn't seem that they directly verify that the HTTPI logger was set correctly. Probably makes more sense to verify that it's the same instance as the Savon logger.

Would you mind making that change before we merge?

@krissi
Copy link
Contributor Author

krissi commented Oct 28, 2014

I have to admit I did not create any spec for this. The changes were only to let the specs pass again.

I could have fixed the specs to output checking instead of message expectations, but there is a great feature in RSpec 3.1 which allows the following, so I skipped the refactoring of those specs

specify { expect { warn('foo') }.to output("foo\n").to_stderr }

@tjarratt
Copy link
Contributor

Thanks for updating your pull request @krissi! I had hoped to get the tests green on travis-ci before merging this, but it's been a real struggle to accomplish that lately. I feel like this change is pretty simple and not too likely to break anyone else, so I'm more than happy to merge this in.

tjarratt added a commit that referenced this pull request Nov 12, 2014
Set HTTPI logger to Savon logger
@tjarratt tjarratt merged commit b77dce3 into savonrb:master Nov 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants