json icon indicating copy to clipboard operation
json copied to clipboard

pretty_generate: maybe ** should be added to state.generate

Open ShadSterling opened this issue 5 years ago • 4 comments

https://github.com/flori/json/blob/da50acc595487c6da9d04011c6b8606be8e54e67/lib/json/common.rb#L289

Ruby 2.7.1 says warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call for some receivers. (In my case for a class that has def to_json *args, **named; (@table.to_json *args, **named); end)

ShadSterling avatar May 13 '20 18:05 ShadSterling

Sorry for the delay in responding.

Thanks for bringing this up. This is a complex issue.

tldnr: One solution for you (using current 2.3.0 release) is to use the following:

def to_json(state = {})
  @table.to_json(**state)
end

This bug should be fixed though. I'm summarizing my findings here:

Firstly, the stacktrace points to state.generate(obj), but that's only because it's the last Ruby code executed; the warning is generated in the C code when state (which is of class JSON::Ext::Generator::State) calls the object's to_json with itself as the only argument.

It's super unclear from the code or the documentation, but currently all calls to_json from the gem will be like that: a single argument which is the current state. I'm not sure why all the code in json/ext/ for example has def to_json(*args) when there will never be more than 1 argument.

Now the tricky part is that State defines to_hash, which allows it to be implicitly converted to a Hash (is that even correct? probably too late to change). In Ruby 2.6 or earlier, this means that:

def foo(*args, **options)
  p args, options
end
state = JSON::Ext::Generator::State.new
foo(state) # => [], {:indent=>"", :space=>"", :space_before=>"", ...

That is, args == [] and options == state.to_hash.

In Ruby 3.0 that won't work and args == [state], options = {} (at least that's the plan, the transition is quite tricky).

I'm a bit out of touch with the threads on keyword conversion (it makes me cringe a bit when I read them), but I think that calling to_json(**state.to_h) internally will work for def to_json(*args) as well as def to_json(*args, **options) and resolve this issue.

so I'm not sure exactly if it's possible / a good idea or how efficient it would be to call to_json(**state.to_hash) internally when the function to_json allows it.

This is further complicated by my ignorance of the state of affairs in the java world; maybe @headius can help on the JRuby side, assuming they are following MRI on keyword conversion...

marcandre avatar Jun 30 '20 06:06 marcandre

All I was trying to do is effectively delegate #to_json to @table (but outside of rails, so I don't have the delegate helper), in a way that's compatible with JSON.pretty_generate. That first attempt I assumed would just pass through all positional and named parameters as received, but it sounds like that's not what it actually does. The solution I'd like for my code is whatever does just pass through all positional and named parameters so I can do that sort of delegation without knowing the actual parameters and without adding ActiveSupport to my dependencies and without having to change anything if the parameters change in future versions. What I ended up doing is def to_json *args, generator_state, **named; @table.to_json *args, generator_state, **named; end, which I haven't noticed any problem with; if I do I'll try your suggestion.

ShadSterling avatar Jun 30 '20 20:06 ShadSterling

Just to be clear, I haven't checked the @table.to_json; maybe it doesn't actually need keyword arguments, in which case the following probably works fine for your use?

def to_json(*args)
  @table.to_json(*args)
end

marcandre avatar Jun 30 '20 20:06 marcandre

@table is just a Hash; the class is based on OpenStruct, which doesn't have a to_json. I want to be able to just pass all parameters through without knowing what the parameters are, so even if they change I don't have to change the passthrough.

ShadSterling avatar Jun 30 '20 21:06 ShadSterling