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

Make ruby-ntlm an optional dependency #108

Merged
merged 4 commits into from
Feb 5, 2014
Merged

Conversation

tjarratt
Copy link
Contributor

@tjarratt tjarratt commented Feb 1, 2014

Decided to bump the minor version number because this is technically a breaking change in the API.

Fixes #107

tjarratt and others added 3 commits January 31, 2014 23:04
Decided to bump the minor version number because this is technically a breaking
change in the API.

Fixes #107
@rogerleite
Copy link
Member

I'm thinking in one scenario. If user doesn't have rubyntlm and do a request with "ntlm" auth, i'm not sure what error message will appears to him.

@tjarratt do you think is too much check if is NTLM is loaded on line:

negotiate_ntlm_auth(http, &requester) if @request.auth.ntlm? # https://github.com/savonrb/httpi/pull/108/files#diff-f34ff4ddc1edbabe503808c45b11cb51L67?

?

@tjarratt
Copy link
Contributor Author

tjarratt commented Feb 4, 2014

@rogerleite That's a great idea. Currently they would see a debug message when net_http.rb is required, but I think improving the error message around the code you pointed out is a good idea.

Tip of the hat to @rogerleite for suggesting this improvement.
@tjarratt
Copy link
Contributor Author

tjarratt commented Feb 5, 2014

Added a fatal log and a small test for this behavior. I felt bad about submitting a PR initially that did not have any tests.

@rogerleite
Copy link
Member

Great:exclamation: :shipit:

BTW, looking at Travis, version 1.8.7 had another problem with dependencies. We should drop this version.

rogerleite added a commit that referenced this pull request Feb 5, 2014
Make ruby-ntlm an optional dependency
@rogerleite rogerleite merged commit b289cb4 into master Feb 5, 2014
@rogerleite rogerleite deleted the ntlm-is-optional branch February 5, 2014 09:58
@tjarratt
Copy link
Contributor Author

tjarratt commented Feb 6, 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.

Remove rubyntlm dependency from gemspec
2 participants