use send_stat instead of overriding ruby send method #13
Conversation
…add prefix method
|
Thanks a lot @sensadrome for your contribution. I really like the code cleaning 😄 but I am not sure to understand what's exactly the issue you're experiencing with On a side note, it seems that these changes break the test suite. |
|
@sensadrome any update about this PR ? |
|
@yannmh sorry - been busy! - so "send" is an inbuilt ruby method on Object (http://ruby-doc.org/core-2.2.2/Object.html#method-i-send) - technically speaking we could use "send" instead which I think another dev suggested. In your code you specify "alias :send :send_to_socket" which effectively overwrites the Object.send method and thus leads to unexpected behaviour.... essentially your send method switches between send_to_socket and send_to_buffer which is nice - but instead of using :send as the alias I have used :send_stat Hope that makes sense! Re the tests, I can see that this is part of the rewrite for the initialiser - so I would have to rewrite the tests. Happy to do so if you think you'd like to include my changes... |
|
@yannmh OK - I have updated the code and adjusted the spec helpers to initialise the object with a hash instead of ordered args. Tests all pass now ... sorry, should have done that before! |
|
should ruby 1.8.7 be removed as a target build, since i18n requires 1.9.3 ? |
| def initialize(host = DEFAULT_HOST, port = DEFAULT_PORT, opts = {}, max_buffer_size=50) | ||
| self.host, self.port = host, port | ||
| @prefix = nil | ||
| def initialize(opts = {}) |
There was a problem hiding this comment.
This is unfortunately breaking backward incompatibility as we are changing the method signature 😢 , i.e.
s = Statsd.new('localhost', 1234)
ArgumentError: wrong number of arguments (2 for 0..1)
|
Thanks a lot for your work and the further information @sensadrome ! We'd like to merge your contribution and release a new gem. Unfortunately some of them are breaking backward incompatibility (i.e. comment made above). |
|
To avoid backward incompatibilities issues, I opened a new PR #17 with your first commit cherry-picked. Thanks again. |
|
yes fair enough - I hadn't thought about backward compatibility. Happy to add the code to cover it if you like - right now we're just using the gem directly from our own repo, but ultimately would be better to use yours :) Let me know... |
|
@sensadrome, that would be great ! Thank you |
in some implementations of the code we might want to call something like:
type = :increment
$statsd.send(type, my_stat)
but this would not work because send has been aliased to one of send_to_buffer or send_to_socket