Rewrite get_data

[pilhuhn]

As discussed in #36 the current impl's set of parameters is overloaded and we should rewrite to pass all optional params in a hash.

[abonas]

while at it, I recommend renaming it to simply 'data' per ruby conventions. (the get/set_X is less popular in ruby)

[jkremser]

I was looking into it and it is actually using hash. There is a difference between

def get_data(id, starts: nil, ends: nil, bucketDuration: nil, buckets: nil, percentiles: nil, limit: nil, order: nil)

and

def get_data(id, starts = nil, ends = nil, bucketDuration = nil, buckets = nil, percentiles = nil, limit = nil, order = nil)

Both methods takes the id as a first param. The difference here is that the second method can accept optional parameters, but it's an ordered list, so if I wan't to specify, say, buckets, but don't want to specify anything else, I have no way to do that.

The second signature is different, the order doesn't matter, because technically the method accept an id and a hash. I can call the first method as

get_data(42, starts: now - (2 *foo), ends: now - foo)

(https://git.io/vre0e)

or even

additional_params = {
  start: foo,
  end: bar,
  order: baz
}
get_data(42, additional_params)

note: Ruby doesn't allow to call the 2nd method this way get_data(42, now - (2 *foo), now - foo). I.e. w/o explicitly naming the params in the hash.

So i think, we are good here as for the hash and passing the arguments. Correct, me if I am wrong or missing something.

So what about the get_data -> data renaming, does it worth to break the api?

[jkremser]

Oh, and it's Ruby 2+ feature, but it shouldn't be the issue (https://robots.thoughtbot.com/ruby-2-keyword-arguments)

[pilhuhn]

So I hear you saying @Jiri-Kremser this is no issue and already works? If so, could you add some tests please that use this alternate style to make sure it continues to work in the future?

[jkremser]

this was the original:

def get_data(id, starts: nil, ends: nil, bucketDuration: nil, buckets: nil)

and this is the new version:

def get_data(id, starts: nil, ends: nil, bucketDuration: nil, buckets: nil, percentiles: nil, limit: nil, order: nil)

(https://github.com/hawkular/hawkular-client-ruby/pull/36/files#diff-a9c2c337e0e2e8129b0179df69635862L119)

This is not a breaking change. I think, it's well tested (https://git.io/vrJIm) but I can add a case for get_data being called with all the possible arguments.

[jkremser]

#65

[pilhuhn]

@abonas "So what about the get_data -> data renaming, does it worth to break the api?"

[abonas]

@pilhuhn regarding breaking the api - it depends how many users the ruby client has. if only 1 and we know who, and it's one place to rename - I'd go ahead and rename it.
in any case there's also an option to do "alias_method" and document the "new" method name, that won't break a thing.