Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ShadSterling opened this issue May 13, 2020 · 4 comments
Open

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

ShadSterling opened this issue May 13, 2020 · 4 comments

Comments

@ShadSterling
Copy link

state.generate(obj)

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)

@marcandre
Copy link
Contributor

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...

@ShadSterling
Copy link
Author

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.

@marcandre
Copy link
Contributor

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

@ShadSterling
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants