Fix an issue with generate_pretty and empty objects in the Ruby and Java implementations #449
Conversation
chrisseaton
force-pushed
the
pretty-empty
branch
2 times, most recently
from
61efee1
to
f5916bd
Compare
|
Also fixes #440 now. |
chrisseaton
changed the title
lib/json/pure/generator.rb
Outdated
| } | ||
| depth = state.depth -= 1 | ||
| result << state.object_nl | ||
| result << state.indent * depth if indent | ||
| unless count.zero? |
There was a problem hiding this comment.
Could we use unless first to handle this instead of adding count?
There was a problem hiding this comment.
I wasn't quite sure about what the 'object' could be and wether it was possibly an enumerator that can only run once?
There was a problem hiding this comment.
first == true is always equivalent to count == 0, isn't it? (they're modified together in the current diff)
There was a problem hiding this comment.
Ah I see what you mean! Yes I'll do that.
There was a problem hiding this comment.
Done - I thought you were suggesting I called .first on the contents.
chrisseaton
force-pushed
the
pretty-empty
branch
2 times, most recently
from
89001db
to
31b9817
Compare
…ava implementations
chrisseaton
force-pushed
the
pretty-empty
branch
from
31b9817
to
b8c1c64
Compare
|
It would be helpful to give a JRuby contributor write access to this repository, if this is where we will continue to manage the json extension. That person should probably be me. |
|
|
||
| final boolean[] firstPair = new boolean[]{true}; | ||
| object.visitAll(new RubyHash.Visitor() { |
There was a problem hiding this comment.
This is fine for the patch, but it's now constructing two objects instead of one. Making this an inner class with an accessible firstPair field would get it back down to one object.
There was a problem hiding this comment.
Ah, read it out of the object field, yeah.
There was a problem hiding this comment.
It turned out to be quite a lot of refactoring to make the field accessible, as the local holding the visitor needs a new named class or interface in order to be able to read a field out of it, so I didn't do this change. I don't know if you want to @headius? Otherwise I'll leave it as is.
|
I believe this has not been released yet... so I am requesting a release! We were unable to resolve jruby/jruby#6338 in JRuby 9.2.14.0 because we do not ship prerelease code for default gems. |
Fixed #441.