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

oroth8 · 2022-08-23T19:21:31Z

Items Addressed

feature request

adds the ability to a true_value option to :boolean parser
true_value can be any single value

Notes for Reviewers

I did not update version, was not sure if we batched approved PRs for release or not
I added several test cases but if I missed any please let me know
Also was not sure about naming the option. I ultimately decided to do true_value but open for suggestions

lib/decanter/parser/boolean_parser.rb

inveterateliterate

@oroth8 some really clever changes and refactors / reusability! Some more trivial questions for you.

lib/decanter/parser/boolean_parser.rb

inveterateliterate · 2022-08-31T01:45:17Z

Also correct me if I'm wrong @chawes13 but I think we generally update versions in each PR even if there are a handful open. I suppose you could update right after approval prior to merge to mitigate multiple commits around versioning.

Gemfile.lock

chawes13 · 2022-09-01T12:47:54Z

README.md

+`: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'
+```
+


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.

chawes13 · 2022-09-01T12:51:32Z

README.md

+`: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'


Out of curiosity, why truth instead of truthy?

just personal preference, but can switch it to truthy

chawes13 · 2022-09-01T12:52:10Z

lib/decanter/parser/boolean_parser.rb

+      end
+
+      def self.normalize(value)
+        raise Decanter::ParseError.new 'Expects a single value' if value.is_a? Array


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)

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.

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

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

chawes13 · 2022-09-01T13:01:17Z

lib/decanter/parser/boolean_parser.rb

+        option_val = options.fetch(:true_value, nil)
+        normalized_option = normalize(option_val)


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?

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.

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.

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

chawes13 · 2022-09-01T13:06:00Z

lib/decanter/parser/boolean_parser.rb

+
+        true_values = ['1', 'true']
+
+        option_val = options.fetch(:true_value, nil)


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)

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

chawes13 · 2022-09-01T13:13:56Z

lib/decanter/parser/boolean_parser.rb

+        normalized_option = normalize(option_val)
+
+        true_values << normalized_option if normalized_option
+        true_values.find {|tv| !!/#{tv}/i.match(normalized_val)}.present?


🍔 (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

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.

chawes13 · 2022-09-01T13:24:01Z

spec/decanter/parser/boolean_parser_spec.rb

+    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})


Out of curiosity, is the older hashrocket syntax required?

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?

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

chawes13 · 2022-09-01T13:25:32Z

spec/decanter/parser/boolean_parser_spec.rb

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


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

In the normalize method we return nil if blank

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

this was part of the originally functionality

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

chawes13 · 2022-09-01T13:28:22Z

spec/decanter/parser/boolean_parser_spec.rb

+    end
+
+    context 'returns false with options for' do
+      falses_with_options.each do |cond|


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.

chawes13 · 2024-01-05T15:00:42Z

@oroth8 Yikes, it's only been a year since I last looked at this 🙈 🙊 . Would you mind resolving the conflicts? Are there any other updates required from the code review feedback?

inveterateliterate · 2024-03-01T22:02:35Z

curious what's open on this one? kinda want to use it 😅

add logic to BooleanParser to allow for optional truthy arguments

ec0b1fe

oroth8 requested a review from chawes13 as a code owner August 23, 2022 19:21

oroth8 marked this pull request as draft August 23, 2022 21:30

oroth8 added 4 commits August 23, 2022 16:32

remove unneeded test cases

64d7bef

refactor single value check, rename arguments

e19773f

update readme

0322c82

add readme discription to right section

be56549

oroth8 linked an issue Aug 23, 2022 that may be closed by this pull request

Pass in explicit truthy / falsy options to transform with boolean parser #131

Open

oroth8 added the enhancement label Aug 23, 2022

oroth8 self-assigned this Aug 23, 2022

oroth8 marked this pull request as ready for review August 23, 2022 22:25

oroth8 requested a review from inveterateliterate August 23, 2022 22:29

inveterateliterate reviewed Aug 29, 2022

View reviewed changes

lib/decanter/parser/boolean_parser.rb Outdated Show resolved Hide resolved

added append to true_values array refactor

c245c0d

oroth8 requested a review from inveterateliterate August 30, 2022 20:35

inveterateliterate reviewed Aug 31, 2022

View reviewed changes

oroth8 added 2 commits August 31, 2022 09:20

more refactors

4cd6574

update version

590fb71

oroth8 requested a review from inveterateliterate August 31, 2022 14:27

inveterateliterate reviewed Aug 31, 2022

View reviewed changes

Gemfile.lock Outdated Show resolved Hide resolved

adjusted version number

6d6d7ba

oroth8 requested a review from inveterateliterate August 31, 2022 19:34

inveterateliterate approved these changes Aug 31, 2022

View reviewed changes

chawes13 reviewed Sep 1, 2022

View reviewed changes

nicoledow mentioned this pull request Dec 4, 2023

134 Enhance ReadMe with documentation on parser options #145

Merged

3 tasks

Merge branch 'main' into feature/allow-truthy-options-for-boolean-parser

71485dc

nicoledow self-requested a review January 3, 2025 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

oroth8 commented Aug 23, 2022 •

edited

Loading

inveterateliterate left a comment

inveterateliterate commented Aug 31, 2022

chawes13 Sep 1, 2022

chawes13 Sep 1, 2022

chawes13 Sep 1, 2022

oroth8 Sep 1, 2022

chawes13 Sep 1, 2022

oroth8 Sep 1, 2022

inveterateliterate Sep 1, 2022 •

edited

Loading

chawes13 Sep 2, 2022

inveterateliterate Sep 2, 2022

chawes13 Sep 1, 2022

oroth8 Sep 1, 2022 •

edited

Loading

inveterateliterate Sep 1, 2022

inveterateliterate Sep 1, 2022

chawes13 Sep 1, 2022

oroth8 Sep 1, 2022

chawes13 Sep 1, 2022

chawes13 Sep 1, 2022

chawes13 Sep 1, 2022

oroth8 Sep 1, 2022

chawes13 Sep 1, 2022

oroth8 Sep 1, 2022

chawes13 Sep 2, 2022

chawes13 Sep 1, 2022

oroth8 Sep 1, 2022

oroth8 Sep 1, 2022

inveterateliterate Sep 1, 2022

chawes13 Sep 1, 2022

chawes13 commented Jan 5, 2024

inveterateliterate commented Mar 1, 2024

		option_val = options.fetch(:true_value, nil)
		normalized_option = normalize(option_val)


		true_values = ['1', 'true']

		option_val = options.fetch(:true_value, nil)

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

Are you sure you want to change the base?

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

Conversation

oroth8 commented Aug 23, 2022 • edited Loading

Items Addressed

Notes for Reviewers

inveterateliterate left a comment

Choose a reason for hiding this comment

inveterateliterate commented Aug 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inveterateliterate Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oroth8 Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chawes13 commented Jan 5, 2024

inveterateliterate commented Mar 1, 2024

oroth8 commented Aug 23, 2022 •

edited

Loading

inveterateliterate Sep 1, 2022 •

edited

Loading

oroth8 Sep 1, 2022 •

edited

Loading