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

Extension to use dry-logic predicates as macros #548

Merged
merged 8 commits into from Jun 27, 2019

Conversation

Projects
None yet
4 participants
@waiting-for-dev
Copy link
Collaborator

commented Jun 13, 2019

PoC for :gteq?

This is the implementation for #537

Some considerations:

  • Right now it just loads with :gteq? predicate. We have to decide which predicates need to be imported. My proposal is:

    • empty?
    • filled?
    • odd?
    • even?
    • lt?
    • gt?
    • lteq?
    • gteq?
    • size?
    • min_size?
    • max_size?
    • inclusion?
    • exclusion?
    • included_in?
    • excluded_from?
    • includes?
    • excludes?
    • eql?
    • is?
    • not_eql?
    • true?
    • false?
    • uuid_v4?
    • respond_to?
  • I think Dry::Validation::Predicate#arg_names and Dry::Validation::Predicate#call should be moved to Dry::Schema::PredicateRegistry.

  • I think it would be nice to keep name and options in the generated error when doing .failure(name, options). If I'm understanding what it is happening, they are just used as an intermediate info to build the error message (must be greater than... in this case). This way I could test what I want to test (that I provide that information) and not have to rely on the message generation, which makes me depend on the yaml, etc..

  • Surely it has been discussed before, but I feel it is a duplication being able to use predicates in dry-schema. I think that dry-schema should only be concerned with structure and types, while dry-validation should deal with values.

Thanks for any feedback.

@waiting-for-dev waiting-for-dev force-pushed the predicates_as_macros branch from 383fc57 to a1e0f47 Jun 13, 2019

@solnic

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Surely it has been discussed before, but I feel it is a duplication being able to use predicates in dry-schema. I think that dry-schema should only be concerned with structure and types, while dry-validation should deal with values.

That's the recommended way of using dry-validation starting from 1.0.0. We don't want to enforce it though. It's just a tool, do whatever you feel is good for you, including shooting yourself in the foot and learning the lesson 😄

@waiting-for-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2019

That's the recommended way of using dry-validation starting from 1.0.0. We don't want to enforce it though. It's just a tool, do whatever you feel is good for you, including shooting yourself in the foot and learning the lesson

I see :) However, being it the recommended way of using dry-validation, wouldn't it make sense to have it as core behavior instead of an extension?

@solnic

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@waiting-for-dev yes but I want to start with the extension and mark it as experimental. I expect things to fully shape up for 2.0.0. Does this make sense?

@waiting-for-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2019

@waiting-for-dev yes but I want to start with the extension and mark it as experimental. I expect things to fully shape up for 2.0.0. Does this make sense?

Yes, better approach it step by step.

@solnic

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Right now it just loads with :gteq? predicate. We have to decide which predicates need to be imported. My proposal is: (...)

This list looks good to me. Are you planning to continue working on this?

@waiting-for-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 24, 2019

This list looks good to me. Are you planning to continue working on this?

Yes, I just wanted some feedback before going forward. I can work on this during this week. What do you think about the other points?:

I think Dry::Validation::Predicate#arg_names and Dry::Validation::Predicate#call should be moved to Dry::Schema::PredicateRegistry.

I could prepare a PR to dry-schema with those two methods added and then continue here.

I think it would be nice to keep name and options in the generated error when doing .failure(name, options). If I'm understanding what it is happening, they are just used as an intermediate info to build the error message (must be greater than... in this case). This way I could test what I want to test (that I provide that information) and not have to rely on the message generation, which makes me depend on the yaml, etc..

This one is not necessary for this PR, but it could be implemented afterwards.

@solnic

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

I think Dry::Validation::Predicate#arg_names and Dry::Validation::Predicate#call should be moved to Dry::Schema::PredicateRegistry.
I could prepare a PR to dry-schema with those two methods added and then continue here.

OK, but why is this refactoring needed?

@waiting-for-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

I think Dry::Validation::Predicate#arg_names and Dry::Validation::Predicate#call should be moved to Dry::Schema::PredicateRegistry.
I could prepare a PR to dry-schema with those two methods added and then continue here.

OK, but why is this refactoring needed?

It is just that I think that it is where these methods belong. It is a responsibility for PredicateRegistry, I'm only dispatching there from here. But if you are in a hurry with this issue I can do it afterwards.

@waiting-for-dev waiting-for-dev force-pushed the predicates_as_macros branch from a1e0f47 to 235f439 Jun 25, 2019

@waiting-for-dev waiting-for-dev force-pushed the predicates_as_macros branch from 235f439 to 7053ba1 Jun 25, 2019

@waiting-for-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

@solnic for now I added missing predicates, so it is complete in that regard.

It seems that :uuid_v4? predicate is not in the dry-logic's version in use, so for now I removed it.

I rebased commits to master.

@flash-gordon

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Why do we use whitelist in the first place?

@solnic

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@flash-gordon can we generate this list in a reliable way?

@flash-gordon

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@solnic what predicates should be excluded?

@solnic

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@flash-gordon actually, stop, let's keep it hardcoded, we want to document it anyway, having an explicit list will be useful and it's not like we will have to change it all the time. WDYT?

@flash-gordon

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

but why not give a link to dry-logic where it should be covered anyway?

@solnic

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@flash-gordon jumping between docs is a bad experience but I don't want to get into details here (I have an idea how to avoid this)

@flash-gordon

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

My only concern was about uuid_v4 which is deliberately excluded, we should bump dry-logic instead. It wouldn't be necessary if we didn't have the whitelist.

@solnic

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@flash-gordon we can add it now and rely on dry-logic's master in the meantime, then just release new dry-validation along with dry-logic

@flash-gordon

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@solnic I released it a week ago or so

@solnic

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@waiting-for-dev could you re-add uuid_v4? it is available in the latest dry-logic (we need to bump min version requirement too)

@waiting-for-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

@waiting-for-dev could you re-add uuid_v4? it is available in the latest dry-logic (we need to bump min version requirement too)

@solnic done. I bumped dry-schema minimal version to 1.2. It is also where dry-logic dependency was bumped for respond_to? predicate.

Why do we use whitelist in the first place?

@flash-gordon my opinion on this is that is better to have a whitelist (explicitness) vs. a blacklist (implicitness). It is possible that another predicate is added to dry-logic and that we don't want it here. The difference is having a feature request from time to time (adding a predicate) vs. having a bug report (a predicate that makes no sense as a validation rule).

@flash-gordon

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

I'd prefer a bug report and a fix w/o either blacklists or whitelists i.e. by differentiating predicates with a ... predicate.

@waiting-for-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

differentiating predicates with a ... predicate.

That would be great. But, how?

@flash-gordon

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Determined by the criteria of what predicates we want to keep. First of all, why not have them all, what are the predicates we don't want to have in dry-v? Anyhow, I'm not against current implementation, no need to change anything about it.

@waiting-for-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

what are the predicates we don't want to have in dry-v?

We don't want to have as predicates for business validation rules those which are not concerned with data but with the type or structure of that data (like :string?). Those predicates have to be used only in the schema definition.

Determined by the criteria of what predicates we want to keep.

The only way I can think is to differentiate between data predicates and type predicates in dry-logic itself. But from dry-logic's point of view I don't know if it is a good idea.

@solnic

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

First of all, why not have them all, what are the predicates we don't want to have in dry-v?

Like @waiting-for-dev said - type checks, because the entire point of having a schema is to NOT have to check for types in the rules. I plan a feature where you'll be able to do pattern-matching in rules that may involve type checks, but that's a whole different story.

@solnic
Copy link
Member

left a comment

I left one comment but overall I'm thinking that maybe we want to have Validation::PredicateRegistry < Schema::PredicateRegistry that would implement whatever is needed and be able to leverage the existing API.

message_opts = arg_names.zip(arg_values).to_h

key.failure(name, message_opts)
end

This comment has been minimized.

Copy link
@solnic

solnic Jun 26, 2019

Member

it would be better to just return message_opts here and rename this method to message_opts and then in import_predicates_as_macros you could just do key.failure(name, predicate.message_opts). Otherwise, Predicate will be coupled to evaluator's key API.

This comment has been minimized.

Copy link
@waiting-for-dev

waiting-for-dev Jun 26, 2019

Author Collaborator

Yeah, much better. Done in last commit.

@waiting-for-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2019

I'm thinking that maybe we want to have Validation::PredicateRegistry < Schema::PredicateRegistry that would implement whatever is needed and be able to leverage the existing API.

Do you mean replacing Validation::Predicate by Validation::PredicateRegirtry < Schema::PredicateRegistry, alongside moving/adapting its methods? I think that it makes sense for #message_opts, but #arg_names & #call would be in Validation::PredicateRegistry just because of circumstantial reasons, as they naturally belong to Schema::PredicateRegistry (and who knows if eventually we'll need them in dry-schema).

Anyway, tell me what you prefer and I'll do it in next commit.

@waiting-for-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2019

I think that it makes sense for #message_opts

Well, right now I haven't check, but I guess dry-validation shares the API of dry-schema in this regard. In this case, I think that #message_opts should go to Schema::PredicateRegistry, too :)

@solnic

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

I think that #message_opts should go to Schema::PredicateRegistry

Let's not touch this for now and just start with Validation::PredicateRegistry. Dealing with message options is extremely tricky on the schema side so I don't want to make any changes related to this as part of this feature.

@waiting-for-dev

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2019

Ok, if I understood well, now it should be correct. Because I understand that, for now, we are going to keep Validation::PredicateRegistry "local" to the extension. This is also the reason why I define WHITELIST constant here and not in Validation::Contract. This way I don't pollute further latter namespace.

waiting-for-dev added some commits Jun 27, 2019

Remove require no longer needed
Ref 548#discussion_r297822105
Use unless to set key failure
Ref 548#discussion_r297823217
@solnic

solnic approved these changes Jun 27, 2019

@solnic solnic merged commit b5f8f53 into master Jun 27, 2019

3 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
codeclimate 1 issue to fix
Details
Hound 1 violation found.
codeclimate/diff-coverage 100% (99% threshold)
Details
codeclimate/total-coverage 99% (0.0% change)
Details

@solnic solnic deleted the predicates_as_macros branch Jun 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.