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

don't pass non-JSON specific producer_args to to_json #64

Closed
wants to merge 2 commits into from
Closed

don't pass non-JSON specific producer_args to to_json #64

wants to merge 2 commits into from

Conversation

djerius
Copy link
Contributor

@djerius djerius commented Aug 4, 2015

The producer_args passed by sqlt contains a number of keys which are
not specific to JSON. These were passed unfiltered to to_json().
JSON (at least as of v2.90) will throw an error if it is passed an
unknown option (it uses the option key as a method name, which leads
to confusing error messages).

It's not straightforward to automatically determine the args supported
by the JSON module, so this simply whitelist the 'pretty', 'indent',
and 'canonical' options.

The producer_args passed by sqlt contains a number of keys which are
not specific to JSON.  These were passed unfiltered to to_json().
JSON (at least as of v2.90) will throw an error if it is passed an
unknown option (it uses the option key as a method name, which leads
to confusing error messages).

It's not straightforward to automatically determine the args supported
by the JSON module, so this simply whitelist the 'pretty', 'indent',
and 'canonical' options.
@ribasushi
Copy link
Contributor

Hi!

The original implementation using JSON.pm was a mistake and should not have been merged in the 1st place. There is a plan (awaiting tuits) to switch to JSON::PP unilaterally. I will hold this PR until the switch takes place, and will reevaluate it then. Should be all done in roughly ~3 weeks.

Cheers!

This pull request was closed.
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

Successfully merging this pull request may close these issues.

None yet

2 participants