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

adds support of explict argument for #rule_error? method #673

Merged
merged 2 commits into from
Nov 11, 2020

Conversation

moofkit
Copy link
Contributor

@moofkit moofkit commented Oct 6, 2020

Closing #658

Adds support for #rule_error? for checking that other rule has error

Example

class PersonContract < Dry::Validation::Contract
  schema do
    required(:email).filled(:string)
    required(:name).filled(:string)
  end

  rule(:name) do
    key.failure('name rule error')
  end

  rule(:email) do
    key.failure('email rule error') if rule_error?(:name)
  end
end

PersonContract.new.call(email: 'bar', name: 'foo').errors.to_h
# {name: ['name rule error'], email: ['email rule error']}

if path.nil?
!key(self.path).empty?
else
result.error?(path)
Copy link
Member

Choose a reason for hiding this comment

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

This checks all errors, not just rule errors, so I'm afraid this doesn't do what we want. One option to make this work would be to add Result#rule_error? that would filter out schema errors and look only at rule errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here that schema and rule errors are mixed in Result#errors set.
So I see two possible solutions:

  1. Filter schema errors keys from errors set and searching in that subset.
  2. Check that there are no schema error with provided key and then look up Result#errors.

The first approach seems to me is a little more straightforward.
Because in second one you need to know that rule dependent on schema and does not evaluate if there are some error.
But the second one was easier to implement 😄

If second approach is not ok for you I will rewrite it.

@moofkit moofkit force-pushed the rule_error_with_args branch from 0404501 to 63449fd Compare October 8, 2020 12:04
@moofkit
Copy link
Contributor Author

moofkit commented Oct 14, 2020

@solnic thanks for your review!

I have fixed issue. Can you please have a look one more time? Or I need to rewrite it with another approach?

@moofkit moofkit requested a review from solnic October 17, 2020 07:40
@solnic solnic added this to the 1.6.0 milestone Nov 11, 2020
@solnic solnic added the feature label Nov 11, 2020
@solnic solnic merged commit 658bc27 into dry-rb:master Nov 11, 2020
@solnic
Copy link
Member

solnic commented Nov 11, 2020

@moofkit hey sorry for the delay, I was on a sick leave. Thanks for working on this PR. As you can see I just merged it and it'll be included in the 1.6.0 release.

@moofkit
Copy link
Contributor Author

moofkit commented Nov 11, 2020

@solnic thanks! Hope your are well for now!

@moofkit moofkit deleted the rule_error_with_args branch November 11, 2020 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants