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

add logic to BooleanParser to allow for optional truthy arguments #132

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,12 @@ Parsers can receive options that modify their behavior. These options are passed
input :start_date, :date, parse_format: '%Y-%m-%d'
```

`:boolean` will parse values of `true` and `1` as truthy. If another value is expected to be truthy, use the option `truth_value` to assign a custom truthy case.

```ruby
input :checkbox, :boolean, truth_value: 'yes'
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why truth instead of truthy?

Copy link
Author

Choose a reason for hiding this comment

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

just personal preference, but can switch it to truthy

```

Comment on lines +188 to +193
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting. It looks like there isn't really a place in the README to document which options are accepted for each parser. I think we can leave this here for now, but I'll create another issue to holistically address adding this missing documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

### Exceptions

By default, `Decanter#decant` will raise an exception when unexpected parameters are passed. To override this behavior, you can change the strict mode option to one of:
Expand Down
19 changes: 16 additions & 3 deletions lib/decanter/parser/boolean_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,22 @@ 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)
normalized_val = normalize(val)
next if normalized_val.nil?

true_values = ['1', 'true']

option_val = options.fetch(:true_value, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the README, you named the option as truth_value. We'll need to update the code or the README to be consistent (and accurate)

Copy link
Author

Choose a reason for hiding this comment

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

yep, this is a mistake. Do you think truthy_value or truthy_values is a better solution?

normalized_option = normalize(option_val)
Comment on lines +13 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why are you only allowing one additional option to be specified? What if we wanted to allow "on" or "T" as well?

Copy link
Author

@oroth8 oroth8 Sep 1, 2022

Choose a reason for hiding this comment

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

the scope of the original issue seemed to indicate adding the ability for one optional value. That was my interpretation of the issue, but I can work on a solution that will allow for additional optional values if desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also took this as a design decision (perhaps implicitly by me and adopted by Owen). I don't wholeheartedly disagree with it as this option is per input and is meant to handle the nuance of a given input. Flexibility vs intentionality, and I can see both options. Either way that implementation detail can probably also be documented for the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

taking the more flexible route is definitely fine, as it obviously also covers the single case


true_values << normalized_option if normalized_option
true_values.find {|tv| !!/#{tv}/i.match(normalized_val)}.present?
chawes13 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

🍔 (food for thought): This can be refactored a bit to deal directly with booleans.

true_values.any? { |tv| /#{tv}/i.match?(normalized_val) }

https://ruby-doc.org/core-2.5.1/Regexp.html#method-i-match-3F
https://ruby-doc.org/core-2.7.0/Array.html#method-i-any-3F

Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing this code has made me realize that there is a defect here (which at this point would probably be considered a breaking change): we aren't matching the entire string (i.e., looking for an exact match) 😱 .

This means,

!!/1/.match("false1") # true
!!/true/.match("this is not true") # true

I'll raise an issue for this separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

good catch

end

def self.normalize(value)
chawes13 marked this conversation as resolved.
Show resolved Hide resolved
return if (value.nil? || value === '')
oroth8 marked this conversation as resolved.
Show resolved Hide resolved
raise Decanter::ParseError.new 'Expects a single value' if value.is_a? Array
oroth8 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this part of the normalize method? I'd prefer to see the two decoupled, so that normalize can more closely follow the Single Responsibility Principle (i.e., be responsible for one thing)

Copy link
Author

Choose a reason for hiding this comment

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

I thought about pulling it out but then we would have to check both val and the option_val separately making it less "dry". I opted for the dry trade off. However, looking at your comments below seems like we wont need to do this if opting for multiple option values.

Copy link
Contributor

@inveterateliterate inveterateliterate Sep 1, 2022

Choose a reason for hiding this comment

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

On a separate note I was flipping through all the parsers and I wonder if there's a bigger refactor available here: ValueParser could handle the array and empty checks. HashParser doesn't inherit from ValueParser (not totally sure why) but perhaps by the same logic ArrayParser could inherit from Base. All the other parsers do these same two checks. I haven't done a deep dive to test that out but wanted to surface

Copy link
Contributor

Choose a reason for hiding this comment

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

I like where your head is at. I'd be open to that refactor

Copy link
Contributor

Choose a reason for hiding this comment

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

value.to_s.downcase
oroth8 marked this conversation as resolved.
Show resolved Hide resolved
end
end
end
Expand Down
46 changes: 44 additions & 2 deletions spec/decanter/parser/boolean_parser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,29 @@
['boolean', true],
['string', 'true'],
['string', 'True'],
['string', 'truE']
]

falses = [
['number', 0],
['number', 2],
['string', '2'],
['boolean', false],
['string', 'tru']
]

trues_with_options = [
['string', 'yes', 'string', 'yes'],
['string', 'Yes', 'string', 'yes'],
['string', 'is true', 'string', 'is true'],
['string', 'is truE', 'string', 'is True'],
['number', 3, 'number', 3],
['number', 3, 'string', '3'],
['string', '3', 'number', 3],
['string', 'false', 'string', 'false'],
['string', 'false', 'string', 'False'],
]

falses_with_options = [
['string', 'yes', 'string', '']
]

let(:name) { :foo }
Expand Down Expand Up @@ -61,5 +75,33 @@
.to raise_error(Decanter::ParseError)
end
end

context 'returns true with options for' do
trues_with_options.each do |cond|
it "#{cond[0]}: #{cond[1]}, option: {#{cond[2]}: #{cond[3]}}" do
expect(parser.parse(name, cond[1], true_value: cond[3])).to match({name => true})
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, is the older hashrocket syntax required?

Copy link
Author

Choose a reason for hiding this comment

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

no it is not, but it seems that this spec file and some others use it. Might be worth updating all tests in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me! Happy to consider that out of scope here

end
end
end

context 'returns false with options for' do
falses_with_options.each do |cond|
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need an array for this or could it be just a single assertion? It looks like you arrived to that conclusion by only having one entry in the array. I think you could make this test more explicit if you kept it just to one entry.

it "#{cond[0]}: #{cond[1]}, option: {#{cond[2]}: #{cond[3]}}" do
expect(parser.parse(name, cond[1], true_value: cond[3])).to match({name => false})
end
end
end

context 'with empty string and empty options' do
it 'returns nil' do
expect(parser.parse(name, '', true_value: '')).to match({name => nil})
end
end
Comment on lines +95 to +99
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting edge case. Would you not expect setting true_value: "" to return true?

Copy link
Author

Choose a reason for hiding this comment

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

In the normalize method we return nil if blank

return if (value.nil? || value.blank?) # line 22

Copy link
Author

Choose a reason for hiding this comment

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

this was part of the originally functionality

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed - this also related to the them of a level of opinion in the parser


context 'with nil and nil options' do
it 'returns nil' do
expect(parser.parse(name, nil, true_value: nil)).to match({name => nil})
end
end
end
end