feature/string-attributes: fixed issue#12 #13
Conversation
There was a problem hiding this comment.
Awesome, thanks! Looks great.
Would you mind adding tests for the #internal_value_of for string values here:
https://github.com/kcning/weka-jruby/blob/feature/string-attributes-issue-12/spec/core/attribute_spec.rb#L67?
Let me know if you need any help (I could also take over the testing part, in case you are short of time).
| # else we add the value to the range and return its new index | ||
| add_string_value(value.to_s) | ||
| end | ||
| end |
There was a problem hiding this comment.
I think the string handling could be moved into a separate private method, in order to keep #internal_value_of as concise as possible. Maybe sth. like index_of_string(value)?
With this, we could have a one-liner as for the other types (I'd use the return here as well, even if it is not necessary):
return index_of_string(value) if string?I would also avoid assignments in if clauses and would rather do it before with a fully readable name, like:
index = index_of_value(value.to_s)
if index == -1
# ...
else
# ...
endThe comments could then be the method’s comment and move out of the body (Should work, right? The new method should be short enough, to get what it refers to)
Could you make these changes? Then we are good to go!
Attribute#add_string_value() should not be called inside Attribute#internal_value_of(). The method call is moved into Instances#instance_from().
|
Oke now we have the spec. Can you please check? |
| else | ||
| value | ||
| end | ||
| end |
There was a problem hiding this comment.
You are right–adding the string value should not be done in the Attribute’s #internal_value_for but already during the creation of the instance. Nice catch!
|
Tests are still missing for the implementation of adding string values–all tests pass if you comment out the added code (https://github.com/paulgoetze/weka-jruby/pull/13/files#diff-bb64f986c10a4d064a282d726d7b6c77R232). So, I will just go ahead and merge this and do the rest of the changes in #12 (I think the easiest is to add a string attribute to the test dataset files to cover all string cases in the tests as well). Thanks again for your contributions! |
|
Thanks! I'm using the gem for some string data mining so I can add the use cases to the spec. If I come across the missing methods for string attributes I can also add it. (What are the missing things, actually?) |
|
Nice, then go ahead, I really appreciate some help here! One missing thing I came across is I also added some todos to #12. I will have a look and let you know, when I spot other stuff. One other thing: could you make sure your code is compliant with default Rubocop recommendations? I used it for this project, yet need to add a hint to the Readme. Further discussion can happen either in #12 or a new PR. Thanks! |
Hi, I finished the missing part! #12