add full spec ext type support to JRuby implementation #91
Conversation
tagomoris
force-pushed
the
ext-type-jruby
branch
from
ee93ad2
to
76ba902
Compare
tagomoris
force-pushed
the
ext-type-jruby
branch
from
29f5164
to
b498368
Compare
|
Almost implementation is already done. |
* and this test exists in cruby/unpacker_spec
|
Mission accomplished! Almost all specs are merged into Tests are failing with jruby-19mode on mac environment because of Travis-CI's any troubles, and with jruby-head for unidentified error. I changed the behavior of JRuby implementation below:
@iconara How do you think about this change? Could you review this? |
| return ExtensionValue.newExtensionValue(runtime, type, payload); | ||
|
|
||
| if (this.registry != null) { | ||
| IRubyObject proc = this.registry.lookup(type); |
There was a problem hiding this comment.
You don't need to prefix all instance variable accesses with this.. It's not that it matters if you do, just that it breaks the style of the rest of the Java code.
|
I've pushed commits against reviews. |
|
Failure on JRuby 9.0.1.0 is from this bug: jruby/jruby#3405 |
tagomoris
changed the title
|
@iconara Could you review this change once more? I didn't change |
| public void allowUnknownExt(boolean allow) { | ||
| this.allowUnknownExt = allow; | ||
| } | ||
|
|
There was a problem hiding this comment.
This should be a constructor argument (same goes for #symbolizeKeys).
There was a problem hiding this comment.
Fixed with review, and pushing
Comments are at best outdated, but most of the time they confuse more than they help. Whenever you feel the urge to write a comment, think about how you can rewrite the code to make it not need a comment to explain what it does.
By extracting the code below a comment to a method that explains what the code does the comment is not needed and the code is self-explaining.
What this comment says is better explained by the exception thrown by the method.
|
I think it's harder to explain than show, so I'm going to push up some code in a separate branch showing you what I mean. I'll try to get it done within the next hour or two. |
…ternals Use Java collections for the registry and cache, and use a class for the four-tuple containing the registered entries.
Make it possible to look up extensions in ExtensionRegistry by type ID, and add the features needed to make it work for the unpacking side of things. Eliminate duplicated code in Factory#registered_types_internal and Unpacker#registered_types_internal by moving it to ExtensionRegistry. There are a few tests that fail because of this change. The reason is that Factory#registered_types_internal, Packer#registered_types_internal and Unpacker#registered_types_internal have inconsistent interfaces. The consumers of these methods expect slightly different things in the returned values. I consider this a bug, but I can't change it without changing the C code, something I'm not comfortable with.
|
My opinion on how this should work is now in #92 |
|
@iconara Thank you! I'm checking it. |
* class is lookup key for Packer (and also key for Hash from registered_types_internal) * typeId is lookup key for Unpacker (and also key for Hash from registered_types_internal)
Because ext types for unpacker may be registered without class (just only with proc)
|
@iconara Your code looks fine for me too. I'll merge this branch into master, and then execute re-indentation with 4 spaces for 1 indent level (same with many Java code, like JRuby). Is it ok for you? |
add full spec ext type support to JRuby implementation
|
Anyway, merged! |
This WIP branch is for #87 and #55.
msgpack-ruby v0.7.0 will be released after this branch merged.