136 refactor value parser #143
Conversation
| @@ -10,7 +10,7 @@ class ArrayParser < ValueParser | |||
| # Fetch parser classes for provided keys | |||
| parse_each = options.fetch(:parse_each, :pass) | |||
| item_parsers = Parser.parsers_for(Array.wrap(parse_each)) | |||
| unless item_parsers.all? { |parser| parser <= ValueParser } | |||
| unless item_parsers.all? { |parser| parser <= ValueParser || parser <= PassParser } | |||
There was a problem hiding this comment.
I had to make PassParser inherit from Base so that it wouldn't throw an error when an array was passed to it. I then had to update this line to pass some specs that said all parsers must inherit from ValueParser.
I'm not totally clear on the purpose of PassParser, but it looks like it is used for values that should not be touched/transformed? If I'm understanding this correctly, it seems fine if it does not inherit from ValueParser and therefore does not run a check to ensure the passed value is not an array.
There was a problem hiding this comment.
It's possible that the PassParser has a defect where it doesn't have that guard clause, but should.
This is from the existing README:
Note: these parsers are designed to operate on a single value, except for :array. This parser expects an array, and will use the parse_each option to call a given parser on each of its elements:
There was a problem hiding this comment.
Based on the tests below from the pass_parser_spec tests, it seems PassParser is intended to accept an array (which conflicts with that blurb from the README):
it 'lets anything through' do
expect(parser.parse(name, '(12)2-21/19.90')).to match({name =>'(12)2-21/19.90'})
end
context 'with array value' do
it 'returns the array value' do
expect(parser.parse(name, ['123'])).to match({name => ['123']})
expect(parser.parse(name, [])).to match({name => []})
end
end
I think the result described by the test seems right - I'd expect to be able to pass any value if I had marked the key as :pass in my decanter. If this is right, maybe the right move is to update the README to indicate PassParser allows arrays?
Or, maybe the intended use for PassParser is to allow elements of an array to be any type (i.e. input :items, :array, parse_each: :pass), in which case we would want PassParser to only allow a singular value. In that case, I could update the specs.
There was a problem hiding this comment.
💭 The second option would be a breaking change
| @@ -10,7 +10,7 @@ class ArrayParser < ValueParser | |||
| # Fetch parser classes for provided keys | |||
| parse_each = options.fetch(:parse_each, :pass) | |||
| item_parsers = Parser.parsers_for(Array.wrap(parse_each)) | |||
| unless item_parsers.all? { |parser| parser <= ValueParser } | |||
| unless item_parsers.all? { |parser| parser <= ValueParser || parser <= PassParser } | |||
There was a problem hiding this comment.
It's possible that the PassParser has a defect where it doesn't have that guard clause, but should.
This is from the existing README:
Note: these parsers are designed to operate on a single value, except for :array. This parser expects an array, and will use the parse_each option to call a given parser on each of its elements:
| @@ -1,6 +1,6 @@ | |||
| module Decanter | |||
| module Parser | |||
| class PassParser < ValueParser | |||
| class PassParser < Base | |||
| super(name, value, options) | ||
| end | ||
|
|
||
| def self.validate_singularity(value) |
There was a problem hiding this comment.
Can / should we make this private?
There was a problem hiding this comment.
I think so. Done!
Items Addressed
Address #136 by DRY-ing up the code that checks whether values passed to parsers other than
ArrayParserare singular values and raises an error if not.How It Works
HashParser,ArrayParsernow inherits directly fromBaseinstead ofValueParser(which in turn inherits fromBase).ValueParseris now responsible for handling single-value parsers. In its parser method it now raises an error if passed an array.Author Checklist
version.rbfollowing versioning guidelines