Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
PATH
remote: .
specs:
decanter (4.0.4)
decanter (4.1.0)
actionpack (>= 4.2.10)
activesupport
rails-html-sanitizer (>= 1.0.4)
Expand Down
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,12 @@ input :start_date, :date, parse_format: '%Y-%m-%d'
| `DateParser`| `parse_format` | `'%m/%d/%Y'`| Accepts any format string accepted by Ruby's `strftime` method
| `DateTimeParser` | `parse_format` | `'%m/%d/%Y %I:%M:%S %p'` | Accepts any format string accepted by Ruby's `strftime` method

`: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
Contributor 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

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@oroth8 Mind adding these details to the table that was added to the README in the PR for the issue Conor linked above?

### 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
13 changes: 13 additions & 0 deletions lib/decanter/parser/boolean_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,19 @@ class BooleanParser < ValueParser
allow TrueClass, FalseClass

parser do |val, options|
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
Contributor 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
Contributor 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

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, since the default includes multiple truthy values, then we should be able to specify one or multiple truthy values when using the custom option.


true_values << normalized_option if normalized_option
true_values.find {|tv| !!/#{tv}/i.match(normalized_val)}.present?
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
Contributor 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)
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)
Expand Down
2 changes: 1 addition & 1 deletion lib/decanter/version.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
module Decanter
VERSION = '4.0.4'.freeze
VERSION = '4.1.0'.freeze
end
47 changes: 44 additions & 3 deletions spec/decanter/parser/boolean_parser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,29 @@
['boolean', true],
['string', 'true'],
['string', 'True'],
['string', 'truE']
]

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

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 @@ -62,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
Contributor 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
Contributor 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
Contributor 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