-
Notifications
You must be signed in to change notification settings - Fork 22
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
fix missing ipaddr require #37
fix missing ipaddr require #37
Conversation
@@ -4,6 +4,7 @@ | |||
require "logstash/namespace" | |||
require "socket" | |||
require "stud/interval" | |||
require "ipaddr" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest removing any other require of ipaddr like https://github.com/logstash-plugins/logstash-input-udp/blob/master/spec/support/client.rb#L3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meh - if you insist I canl do it but OTOH it's totally ok to require a lib that is used in the file. For example, if udp.rb
stops using it but it's still in use in client.rb
then it won't break. In fact I'd argue that this is preferable actually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually ... doing that risks reproducing the exact same problem we are solving here :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my argument comes from ipaddr now being a runtime dependency in the gemspec so it must be required in the "lib/" files not the "spec"
That said I'm not blocking this PR b/c of this, up to you.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
« ipaddr now being a runtime dependency in the gemspec so it must be required in the "lib/" files not the "spec"» not following what you mean!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ipaddr now being a runtime dependency in the gemspec
I forgot ipaddr is stdlib, but my argument was that any runtime dependency should be included in the lib/logstash/inputs/udp.rb
, which means we'd never need to include in the specs.
I'm still LGTM on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think that not having an explicit require "somelib"
in a file that explicitly uses Someclass
is an anti-pattern. In this case the IPAddr
class in client.rb
is used independently for its usage in udp.rb
and if one day IPAddr
is not used anymore in udp.rb
for whatever reason, that does not mean that IPAddr
will not be used in client.rb
and following your suggestion of removing require "ipaddr"
from client.rb
it would result in a bug when running specs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, agreed.
@@ -4,6 +4,7 @@ | |||
require "logstash/namespace" | |||
require "socket" | |||
require "stud/interval" | |||
require "ipaddr" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my argument comes from ipaddr now being a runtime dependency in the gemspec so it must be required in the "lib/" files not the "spec"
That said I'm not blocking this PR b/c of this, up to you.
LGTM
merged and published as v3.3.2. |
The
IPAddr
class is used but theipaddr
library is never required. If no other plugins does arequire "ipaddr"
then the udp input will crash on startup with:This fix is simply to require that library.