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.

Sign up for GitHub

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

Make Result#error?(key) return true for rule and schema errors #656

Merged
merged 2 commits into from
Jul 22, 2020
Merged

Make Result#error?(key) return true for rule and schema errors #656

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
232a983
Make Result#error?(key) return true for rule and schema errors
PragTob Jul 17, 2020
4288fae
Address PR feedback :tada:
PragTob Jul 21, 2020
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
4 changes: 2 additions & 2 deletions lib/dry/validation/contract.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,14 @@ def error?(result, spec)
return path.expand.any? { |nested_path| error?(result, nested_path) }
end

return true if result.error?(path)
return true if result.schema_error?(path)

path
.to_a[0..-2]
.any? { |key|
curr_path = Schema::Path[path.keys[0..path.keys.index(key)]]

return false unless result.error?(curr_path)
return false unless result.schema_error?(curr_path)

result.errors.any? { |err|
(other = Schema::Path[err.path]).same_root?(curr_path) && other == curr_path
Expand Down
2 changes: 1 addition & 1 deletion lib/dry/validation/evaluator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ def key?
#
# @api public
def schema_error?(path)
result.error?(path)
result.schema_error?(path)
end

# Check if there are any errors on the current rule
Expand Down
9 changes: 8 additions & 1 deletion lib/dry/validation/result.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,15 @@ def failure?

# Check if values include an error for the provided key
#
# @api private
# @api public
PragTob marked this conversation as resolved.
Show resolved Hide resolved
def error?(key)
errors.any? { |msg| Schema::Path[msg.path].include?(Schema::Path[key]) }
end

# Check if the base schema (without rules) includes an error for the provided key
#
# @api private
def schema_error?(key)
schema_result.error?(key)
end

Expand Down
2 changes: 1 addition & 1 deletion lib/dry/validation/rule.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def each(*macros, &block)
values[root].each_with_index do |_, idx|
path = [*Schema::Path[root].to_a, idx]

next if result.error?(path)
next if result.schema_error?(path)

evaluator = with(macros: macros, keys: [path], &block)

Expand Down
27 changes: 27 additions & 0 deletions spec/integration/result_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,33 @@
end
end

describe "#error?" do
subject { result }
PragTob marked this conversation as resolved.
Show resolved Hide resolved

let(:schema_result) do
double(:schema_result, message_set: [], to_h: {email: "jane@doe.org"})
end

let(:result) do
Dry::Validation::Result.new(schema_result) do |r|
r.add_error(Dry::Validation::Message.new("root error", path: [nil]))
r.add_error(Dry::Validation::Message.new("email error", path: [:email]))
end
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 is basically copy & past from above. Not sure what feelings are around here for contexts and distance to lets :)

end

it "reports an error on email" do
expect(subject.error?(:email)).to eq true
PragTob marked this conversation as resolved.
Show resolved Hide resolved
end

it "reports an error on 'root' when asked for [nil]" do
expect(subject.error?([nil])).to eq 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.

I added this because it's current behavior (given the root error path that I just copied from the other test). Not sure if it should be like that or if there's a nicer way?

Copy link
Member

Choose a reason for hiding this comment

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

Yes base errors are represented by nil key, doing error?(nil) would work too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I double checked and error?(nil) doesn't work.

  1) Dry::Validation::Result#error? reports an error on 'root' when asked for nil
     Failure/Error: errors.any? { |msg| Schema::Path[msg.path].include?(Schema::Path[key]) }
     
     ArgumentError:
       +spec+ must be either a Symbol, Array, Hash or a Path
     # ./lib/dry/validation/result.rb:106:in `block in error?'
     # ./lib/dry/validation/result.rb:106:in `any?'
     # ./lib/dry/validation/result.rb:106:in `error?'
     # ./spec/integration/result_spec.rb:70:in `block (3 levels) in <top (required)>'

Finished in 0.61031 seconds (files took 0.60271 seconds to load)
184 examples, 1 failure

Failed examples:

rspec ./spec/integration/result_spec.rb:69 # Dry::Validation::Result#error? reports an error on 'root' when asked for nil

Should it work? Then I'll see how it could be made to work but it seems like that might be in Schema::Path[key] then aka in dry-schema :)

Copy link
Member

Choose a reason for hiding this comment

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

@PragTob ah right, I forgot that Path doesn't turn nil into [nil] automatically, so yes, this is expected. BTW - for base errors (aka root errors), we have Result#base_error? method.

PragTob marked this conversation as resolved.
Show resolved Hide resolved
end

it "doesn't report errors on non existing keys" do
expect(subject.error?(:nonexistant)).to eq false
PragTob marked this conversation as resolved.
Show resolved Hide resolved
end
end

describe "#inspect" do
let(:params) do
double(:params, message_set: [], to_h: {})
Expand Down