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

Fix rule.each on optional value #709

Merged
merged 2 commits into from
May 28, 2022
Merged

Fix rule.each on optional value #709

merged 2 commits into from
May 28, 2022

Conversation

bautrey37
Copy link
Contributor

@bautrey37 bautrey37 commented May 10, 2022

Skip processing rule.each on optional value when input is nil
When a key has a maybe macro, nil values are valid. However, if a
rule.each is specified on that key, an error is throw saying:

`block in each': undefined method `each_with_index' for nil:NilClass (NoMethodError)

@bautrey37 bautrey37 requested a review from solnic as a code owner May 10, 2022 08:17
@bautrey37
Copy link
Contributor Author

I am now getting this rubocop warning after the change.

lib/dry/validation/rule.rb:79:7: C: Metrics/AbcSize: Assignment Branch Condition size for each is too high. [<8, 24, 6> 26/25]

I'm not sure what I can do to fix it.

@solnic
Copy link
Member

solnic commented May 11, 2022

Thanks for opening this PR. There was an issue with our CI setup in main, I just fixed it so could you rebase this branch and f-push? Also, don't worry about that rubocop warning.

When a key has a `maybe` macro, nil values are valid. However, if a
`rule.each` is specified on that key, an error is throw saying:

```
`block in each': undefined method `each_with_index' for nil:NilClass (NoMethodError)
```
@esparta
Copy link
Contributor

esparta commented May 26, 2022

Sometimes Rubocop is very picky and can be configure to have that ABC rule less problematic
Maybe is also time to reduce the amount of deepness in the method, from proc -> unless -> each_with_index to proc -> each_with_index

index bc05267..45ba724 100644
--- a/lib/dry/validation/rule.rb
+++ b/lib/dry/validation/rule.rb
@@ -82,16 +82,16 @@ module Dry
         @keys = []

         @block = proc do
-          unless result.base_error?(root) || !values.key?(root)
-            values[root].each_with_index do |_, idx|
-              path = [*Schema::Path[root].to_a, idx]
+          next if skip_values_evaluation?(root)

-              next if result.schema_error?(path)
+          values[root].each_with_index do |_, idx|
+            path = [*Schema::Path[root].to_a, idx]

-              evaluator = with(macros: macros, keys: [path], index: idx, &block)
+            next if result.schema_error?(path)

-              failures.concat(evaluator.failures)
-            end
+            evaluator = with(macros: macros, keys: [path], index: idx, &block)
+
+            failures.concat(evaluator.failures)
           end
         end

@@ -100,6 +100,15 @@ module Dry
         self
       end

+      # Helper to determine the need for value's review
+      #
+      # @api private
+      def skip_values_evaluation?(root)
+        result.base_error?(root) || !values.key?(root) || !values.key?(root)
+      end

@solnic solnic merged commit 10a9d68 into dry-rb:main May 28, 2022
@solnic
Copy link
Member

solnic commented May 28, 2022

@esparta thanks, we can address the metric separately from this PR though 😄

@bautrey37
Copy link
Contributor Author

Thanks for merging this PR out! ❤️

I agree, the nesting is quite deep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants