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.

Sign up for GitHub

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

136 refactor value parser #143

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

136 refactor value parser #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
acb696c
move singularity check to ValueParser class
nicoledow Dec 1, 2023
b974b9c
move singularity check to valueparser and have arrayparser inherit fr…
nicoledow Dec 1, 2023
9c59b11
in spec, do not require PassParser to be a subclass of ValueParser
nicoledow Dec 1, 2023
086d452
update version
nicoledow Dec 1, 2023
2a9f417
formatting
nicoledow Dec 1, 2023
1fbf568
bundle
nicoledow Dec 1, 2023
161a77c
Merge branch 'main' into 136-refactor-value-parser
nicoledow Jan 11, 2024
096e281
re-bundle as 4.0.5
nicoledow Jan 11, 2024
d8f7f0e
make validate_singularity a private method
nicoledow Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
PATH
remote: .
specs:
decanter (4.0.4)
decanter (4.0.5)
actionpack (>= 4.2.10)
activesupport
rails-html-sanitizer (>= 1.0.4)
Expand Down
4 changes: 2 additions & 2 deletions lib/decanter/parser/array_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module Decanter
module Parser
class ArrayParser < ValueParser
class ArrayParser < Base

DUMMY_VALUE_KEY = '_'.freeze

Expand All @@ -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 }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@nicoledow nicoledow Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 The second option would be a breaking change

raise Decanter::ParseError.new 'parser(s) for array items must subclass ValueParser'
end
# Compose supplied parsers
Expand Down
1 change: 0 additions & 1 deletion lib/decanter/parser/boolean_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ class BooleanParser < ValueParser
allow TrueClass, FalseClass

parser do |val, options|
raise Decanter::ParseError.new 'Expects a single value' if val.is_a? Array
next if (val.nil? || val === '')
[1, '1'].include?(val) || !!/^true$/i.match?(val.to_s)
end
Expand Down
4 changes: 4 additions & 0 deletions lib/decanter/parser/core.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ def self.included(base)

module ClassMethods

def _parse(name, value, options={})
{ name => @parser.call(value, options) }
end

# Check if allowed, parse if not
def parse(name, value, options={})
if allowed?(value)
Expand Down
1 change: 0 additions & 1 deletion lib/decanter/parser/date_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ class DateParser < ValueParser
allow Date

parser do |val, options|
raise Decanter::ParseError.new 'Expects a single value' if val.is_a? Array
next if (val.nil? || val === '')
parse_format = options.fetch(:parse_format, '%m/%d/%Y')
::Date.strptime(val, parse_format)
Expand Down
1 change: 0 additions & 1 deletion lib/decanter/parser/datetime_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ class DateTimeParser < ValueParser
allow DateTime

parser do |val, options|
raise Decanter::ParseError.new 'Expects a single value' if val.is_a? Array
next if (val.nil? || val === '')
parse_format = options.fetch(:parse_format, '%m/%d/%Y %I:%M:%S %p')
::DateTime.strptime(val, parse_format)
Expand Down
1 change: 0 additions & 1 deletion lib/decanter/parser/float_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ class FloatParser < ValueParser
allow Float, Integer

parser do |val, options|
raise Decanter::ParseError.new 'Expects a single value' if val.is_a? Array
next if (val.nil? || val === '')
val.scan(REGEX).join.try(:to_f)
end
Expand Down
1 change: 0 additions & 1 deletion lib/decanter/parser/integer_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ class IntegerParser < ValueParser
allow Integer

parser do |val, options|
raise Decanter::ParseError.new 'Expects a single value' if val.is_a? Array
next if (val.nil? || val === '')
val.is_a?(Float) ?
val.to_i :
Expand Down
2 changes: 1 addition & 1 deletion lib/decanter/parser/pass_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module Decanter
module Parser
class PassParser < ValueParser
class PassParser < Base
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above


parser do |val, options|
next if (val.nil? || val == '')
Expand Down
1 change: 0 additions & 1 deletion lib/decanter/parser/phone_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ class PhoneParser < ValueParser
allow Integer

parser do |val, options|
raise Decanter::ParseError.new 'Expects a single value' if val.is_a? Array
next if (val.nil? || val === '')
val.scan(REGEX).join.to_s
end
Expand Down
1 change: 0 additions & 1 deletion lib/decanter/parser/string_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ module Decanter
module Parser
class StringParser < ValueParser
parser do |val, options|
raise Decanter::ParseError.new 'Expects a single value' if val.is_a? Array
next if (val.nil? || val === '')
next val if val.is_a? String
val.to_s
Expand Down
10 changes: 9 additions & 1 deletion lib/decanter/parser/value_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,16 @@
module Decanter
module Parser
class ValueParser < Base

def self._parse(name, value, options={})
{ name => @parser.call(value, options) }
self.validate_singularity(value)
super(name, value, options)
end

private

def self.validate_singularity(value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can / should we make this private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. Done!

raise Decanter::ParseError.new 'Expects a single value' if value.is_a? Array
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/decanter/version.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
module Decanter
VERSION = '4.0.4'.freeze
VERSION = '4.0.5'.freeze
end