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

Check rubyntlm version in a 0.4.0+ compatible way #111

Merged
merged 1 commit into from
Mar 26, 2014

Conversation

carlzulauf
Copy link
Contributor

Fixes errors caused by using rubyntlm v0.4.0. Continues to work with v0.3.2+.

rogerleite added a commit that referenced this pull request Mar 26, 2014
Check rubyntlm version in a 0.4.0+ compatible way
@rogerleite rogerleite merged commit 222497b into savonrb:master Mar 26, 2014
@rogerleite
Copy link
Member

Thanks and sorry for the inconvenience! 👍

@@ -7,6 +7,7 @@

begin
require 'net/ntlm'
require 'net/ntlm/version' unless Net::NTLM.const_defined?(:VERSION)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems really weird to me. Is there a reason why the rubyntlm gem doesn't require its own version file itself when you require 'net/ntlm'? Seems very odd that anyone using ntlm would need to require that themselves.

Also, is there a reason to guard against doing this when the constant is already defined?

Sorry for all of the questions, I'm just curious -- I don't normally see a lot of ruby in this style.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why rubyntlm v0.4.0 doesn't require its own version constant. It was moved there pretty recently and is required explicitly by the gemspec.

The guard against the constant already defined is to make sure this also works with rubyntlm v0.3.x, which defines VERSION in net/ntlm and does not have a net/ntlm/version file, so this require would fail without the guard in 0.3.x.

I don't know if I like this style either, but since it was broken in 0.4.0 this was a simple solution to work with both versions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh well that's really annoying. Thanks for the quick response @carlzulauf. Looks like this is the best choice for compatibility between v0.3 and v0.4.

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.

3 participants