-
Notifications
You must be signed in to change notification settings - Fork 5
add logic to BooleanParser to allow for optional truthy arguments #132
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
base: main
Are you sure you want to change the base?
Changes from all commits
ec0b1fe
64d7bef
e19773f
0322c82
be56549
c245c0d
4cd6574
590fb71
6d6d7ba
71485dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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' | ||
| ``` | ||
|
|
||
|
Comment on lines
+188
to
+193
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the README, you named the option as
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, this is a mistake. Do you think |
||
| normalized_option = normalize(option_val) | ||
|
Comment on lines
+13
to
+14
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? | ||
chawes13 marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") # trueI'll raise an issue for this separately.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||
| 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) | ||
|
|
||
| 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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 } | ||
|
|
@@ -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}) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity, is the older hashrocket syntax required?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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| | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an interesting edge case. Would you not expect setting
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the return if (value.nil? || value.blank?) # line 22
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was part of the originally functionality
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
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.
Out of curiosity, why
truthinstead oftruthy?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 personal preference, but can switch it to
truthyThere 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 vote for
truthy