-
Notifications
You must be signed in to change notification settings - Fork 5
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
base: main
Are you sure you want to change the base?
Changes from 5 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 |
---|---|---|
|
@@ -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' | ||
``` | ||
|
||
Comment on lines
+188
to
+193
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. 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. |
||
### 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 |
---|---|---|
|
@@ -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], | ||
] | ||
|
||
falses_with_options = [ | ||
['string', 'false', 'string', 'false'], | ||
['string', 'false', 'string', 'False'], | ||
['string', 'yes', 'string', ''], | ||
] | ||
|
||
let(:name) { :foo } | ||
|
@@ -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}) | ||
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? 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? 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| | ||
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
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 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 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 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
truth
instead 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
truthy