-
Notifications
You must be signed in to change notification settings - Fork 4
add json parser #140
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
add json parser #140
Conversation
| module Decanter | ||
| module Parser | ||
| class JsonParser < ValueParser | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since json is not a native Ruby data type, there is no specific allowed types. Instead all values passed to this parser will be parsed by JSON.parse(val), unless they are nil or blank strings.
lib/decanter/parser/json_parser.rb
Outdated
| class JsonParser < ValueParser | ||
|
|
||
| parser do |val, options| | ||
| raise Decanter::ParseError.new 'Expects a JSON string' if val.is_a?(Array) || val.is_a?(Hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON.parse only accepts string type as an argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive my ignorance here – does ActiveRecord require that the input to create/update is a hash? Or will it accept a serialized JSON string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of checking for an Array or a Hash as opposed to is_a? String?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
I'm having a hard time finding exact documentation, but I don't think there is a specific requirement for it to be hash. Mainly because JSON can be an array, hash, string
-
I think I had it this way just for readability as to avoid
unless, but looking at it again I'm not sure that's any more readable. I think I'll change it tounless is_a? String
This line in particular is difficult since JSON can be many things. I almost decided to remove this entirely, but wanted to keep some error message here. I could make an argument for having no error message here, but that felt wrong?
I leaned into the fact the client will send it as a string, I will check that, and then the code on line 8 will parse it to become a JSON object for Rails to use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From some quick testing in the console, it looks like JSON.parse throws an error for anything that's not a string (including booleans, integers, etc).
Because of that, to me the most readable option seems like unless is_a? String, but I also wonder if a readable alternative could be rescuing from a JSON.parse error to tell the dev that the value is not valid JSON.
lib/decanter/version.rb
Outdated
| @@ -1,3 +1,3 @@ | |||
| module Decanter | |||
| VERSION = '4.0.1'.freeze | |||
| VERSION = '4.0.2'.freeze | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure on versioning semantics for minor feature. I suppose this could be 4.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a backwards compatible feature, which warrants a minor bump (i.e., 4.1.0)
|
I had trouble updating the versioning for this. I updated |
| @@ -0,0 +1,12 @@ | |||
| module Decanter | |||
| module Parser | |||
| class JsonParser < ValueParser | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should inherit from HashParser
See https://github.com/LaunchPadLab/decanter#custom-parser-base-classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that doesn't account for the fact that it could be an array. NVM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea it turns out that JSON is anything lol
lib/decanter/parser/json_parser.rb
Outdated
| class JsonParser < ValueParser | ||
|
|
||
| parser do |val, options| | ||
| raise Decanter::ParseError.new 'Expects a JSON string' if val.is_a?(Array) || val.is_a?(Hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of checking for an Array or a Hash as opposed to is_a? String?
nicoledow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dschadd Bumping - are you still interested and do you still have capacity to address the comments below to get this into an upcoming release? Aiming for the end of the quarter.
| describe '#parse' do | ||
| it 'parses string value and returns a parsed JSON' do | ||
| expect(parser.parse(name, '{"key": "value"}')).to match({name => {"key" => "value"}}) | ||
| end | ||
|
|
||
| context 'with empty string' do | ||
| it 'returns nil' do | ||
| expect(parser.parse(name, '')).to match({name => nil}) | ||
| end | ||
| end | ||
|
|
||
| context 'with nil' do | ||
| it 'returns nil' do | ||
| expect(parser.parse(name, nil)).to match({name => nil}) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned here, I'd love to see a descriptive error thrown if the string can't be parsed as valid JSON. I think that would also warrant a test case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might also be worth adding test cases to ensure a JSON array string (i.e. '['investment', 'early_retirement', 'saving']') and an object ('{ "first_name": "Daniel", "last_name": "Schadd" }' can both be handled.
Hey @nicoledow - don't think I'll be able to get to one this quarter, but would be happy to throw this on my Q3 goals? |
@dschadd No worries! I just wanted to get an idea of what might be in the Q1 release. Next quarter is great! |
| - [Decanter](#decanter) | ||
| - [Migration Guides](#migration-guides) | ||
| - [Contents](#contents) | ||
| - [Basic Usage](#basic-usage) | ||
| - [Decanters](#decanters) | ||
| - [Generators](#generators) | ||
| - [Decanters](#decanters-1) | ||
| - [Parsers](#parsers) | ||
| - [Resources](#resources) | ||
| - [Decanting Collections](#decanting-collections) | ||
| - [Control Over Decanting Collections](#control-over-decanting-collections) | ||
| - [Nested resources](#nested-resources) | ||
| - [Default parsers](#default-parsers) | ||
| - [Parser options](#parser-options) | ||
| - [Exceptions](#exceptions) | ||
| - [Advanced Usage](#advanced-usage) | ||
| - [Custom Parsers](#custom-parsers) | ||
| - [Custom parser methods](#custom-parser-methods) | ||
| - [Custom parser base classes](#custom-parser-base-classes) | ||
| - [Squashing inputs](#squashing-inputs) | ||
| - [Chaining parsers](#chaining-parsers) | ||
| - [Requiring params](#requiring-params) | ||
| - [Default values](#default-values) | ||
| - [Global configuration](#global-configuration) | ||
| - [Contributing](#contributing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just formatting
kweingart08
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicoledow - This looks good to me.
Items Addressed
jsonto be passed to Decanter without a custom parser.jsonorjsonbjsonblob data sent over from UI to a Rails JSON API. It is column specific.Author Checklist
version.rbfollowing versioning guidelines