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

Type specs with filter rules #11

Merged
merged 17 commits into from Jan 24, 2019

Conversation

@solnic
Copy link
Member

commented Nov 17, 2018

This adds the ability to pass type specs as the first argument in #value and #filled or via explicit #type method in DSL. It also introduces #filter method which can be used to define pre value coercion rules.

API

Dry::Schema.define do
  required(:age).filter(format?: /\d+/).value(:int, gt?: 18)
  # or
  required(:age).filter(format?: /\d+/).type(:int).value(gt?: 18)
end

This will:

  1. Check format?
  2. Coerce to :int
  3. Apply value rules

Summary of changes

  • A special input_schema was added to the DSL, it is used by Key macros (required/optional) to define filter rules using the new #filter method
  • If input_schema has any rules, its definition will be added to the processor right after key coercion
  • Key macro overrides #value and #filled to support type specs
  • Result is now created by the processor and it's passed to its steps, if a step returns a hash back, result's output will be replaced by it. This is mostly a performance tweak so that we're not creating multiple results as it's an expensive object. After processing is done, a frozen result instance is returned.
@Morozzzko

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

One of the things that made it harder to persuade my colleagues to use dry-validation is the feeling of redundancy. Let me elaborate on that.

You see, in the former type-specs DSL we would write required(:age, :integer).value(:int?), which made people confused: why would we say that we want an integer two times?

Now the DSL seems to be getting even more complicated: the line required(:age).type(:int).input(format?: /d+/).value(:int?, gt?: 18) specifies an integer three times! That's a lot!

Perhaps, :int? is redundant. Why would you check an :int if it's an int??

So, we could probably live with something like this:

required(:age).input(format?: /d+/\).type(:int).value(gt?: 18)

Alright, we've got rid of a redundant check. Now there's another problem for people: we have two methods that perform similar task — they specify the output type.

I see that an :int which is greater than 18 is just a constrained / dependent type. We should use a single method for that.

I don't know what name is better, but let's say we use output instead of value for the sake of semantics.

required(:age).input(format?: /d+/\).output(:integer, gt?: 18)

required(:age).input(format?: /d+/\).output(Types::Strict::Integer.constrained(gt?: 18))

Alright. This is probably better, but now we've got another problem — we used to have three methods for extra checks:

  • value
  • filled
  • maybe

The truth is: we don't need that much. It's confusing. Not to me — but other developers. I propose we get rid of them.

Steps to do so:

  • Replace value with output
  • Replace filled with output(:filled?) ← going back to the origins, eh
  • Replace maybe with output(:integer, :nil), output(:integer, :nilable) or output(:integer, :optional)

TL;DR

Original dry-validation DSL is confusing to a lot of people which makes it hard to promote the schema.

Main points of confusion:

  • Perceived redundancy
  • Many ways to achieve the same result

I suggest we tackle the problem by:

  • Getting rid of filled and maybe methods — those could be replaced with a single value with extra predicates and type specs
  • Specifying the output type and the constraints in the same place, as opposed to using type(...).value(...)
  • Renaming value into something more apparent, like output

Basically, we transform our code like this:

# before

Dry::Schema.define do
  required(:age).type(:int).input(format?: /d+/).value(:int?, gt?: 18)
end

# after
Dry::Schema.define do
  required(:age).input(format?: /d+/).output(:integer, gt?: 18)
end

With optional types:

# before:
Dry::Schema.define do
  required(:age).type(:int, :nil).input(format?: /d+/).maybe(:int?, gt?: 18)
end

# after: 
Dry::Schema.define do
  required(:age).input(format?: /d+/).output(:integer, :optional, gt?: 18) # or (:integer, :nil) / (:integer, :nilable)
end
@solnic

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2018

@Morozzzko thanks for your feedback, this is really appreciated. I agree that making the API as small and easy to understand as possible is important. Unfortunately, in practice, you simply have so much duplication in schemas that more methods turned out to be very, very useful (ie value(:int?, :filled?) vs filled(:int?)). We can address this separately though (I know it's related to what I'm doing in this PR but I just don't want to tackle too much stuff in one PR, and this will be big anyway).

As far as type spec and input rules go, I should mention that input rules are not required, they are optional, and people actually should not use them unless it's really beneficial. So ie this means using output is not a good idea, because without input rules it will look weird and confusing.

How about this:

# without input rules
required(:age).type(:int).value(gt?: 18)

# with input rules
required(:age).type(:int).coerce_if(format?: /d+/).value(gt?: 18)
@solnic

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2018

...so, forget my idea - @flash-gordon reminded me of his idea that I really liked, and this is how it looks now:

required(:age).filter(format?: /\d+/).value(:int, gt?: 18)

The type spec is still optional, this is something we have to figure out before the release.

@solnic solnic force-pushed the type-specs-with-output-rules branch from a200087 to 7325ba7 Dec 1, 2018

@solnic solnic referenced this pull request Jan 23, 2019
4 of 4 tasks complete

@solnic solnic force-pushed the type-specs-with-output-rules branch from 9157ea4 to 1c9f3fc Jan 23, 2019

@solnic solnic changed the title [WIP] type specs with output rules Type specs with filter rules Jan 23, 2019

@solnic solnic requested a review from timriley Jan 23, 2019

@solnic

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

@timriley this is now updated and ready for a review

@solnic solnic force-pushed the type-specs-with-output-rules branch from d0797f4 to 21bf17f Jan 23, 2019

lib/dry/schema/result.rb Outdated Show resolved Hide resolved
lib/dry/schema/result.rb Outdated Show resolved Hide resolved
@timriley
Copy link
Member

left a comment

@solnic I love this! Really solid work, it was an inspiring PR to read through.

That said, I did have some pretty existential questions arise at certain points; see my inline comments.

Now for a couple of other general thoughts/questions:

  • Am I right to assume that our plan would be to extend this work (once its merged) and try to support inferring a type spec from the first of the list of predicates supplied for a value? e.g. for key(:age).required(:int?, gt?: 18), we could strip the ? off :int? and see if the resulting string matches any of our built-in types?
  • I know it's not part of this PR, but this is the first time I noticed it... and I really love what you did to support something like array[:int] being used for a type spec. This is way more intention-revealing than the old [:int] structure we had to use in dry-v. Kudos! I was wondering if we could take this a step further, and support something like nullable[:int] as a kind of type spec? (maybe[:int] would perhaps be even better but I think that could lead to a bit of confusion given the existence of the maybe value-rules macro?)
lib/dry/schema/key_coercer.rb Show resolved Hide resolved
lib/dry/schema/macros/key.rb Outdated Show resolved Hide resolved
lib/dry/schema/result.rb Outdated Show resolved Hide resolved

@solnic solnic force-pushed the type-specs-with-output-rules branch 2 times, most recently from 44b7b23 to faecf0d Jan 24, 2019

@solnic

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2019

@solnic I love this! Really solid work, it was an inspiring PR to read through.

oh thank you ❤️

Am I right to assume that our plan would be to extend this work (once its merged) and try to support inferring a type spec from the first of the list of predicates supplied for a value? e.g. for key(:age).required(:int?, gt?: 18), we could strip the ? off :int? and see if the resulting string matches any of our built-in types?

Yes, this is the very first I will do after this is merged.

I know it's not part of this PR, but this is the first time I noticed it... and I really love what you did to support something like array[:int] being used for a type spec. This is way more intention-revealing than the old [:int] structure we had to use in dry-v. Kudos!

Ah thanks, I'm glad you like it.

I was wondering if we could take this a step further, and support something like nullable[:int] as a kind of type spec? (maybe[:int] would perhaps be even better but I think that could lead to a bit of confusion given the existence of the maybe value-rules macro?)

Yes this would be a great improvement, it's a matter of deciding on the name of the method. People do confuse element order in the array, ie [:str, :nil] is not the same as [:nil, :str].

solnic added some commits Oct 26, 2018

Introduce input rules
This adds `input` method to the DSL, which allows you to define rules
that will be applied before coercion takes place

solnic added some commits Jan 24, 2019

@solnic solnic force-pushed the type-specs-with-output-rules branch from faecf0d to ebebbf0 Jan 24, 2019

@solnic solnic merged commit d78c229 into master Jan 24, 2019

5 of 6 checks passed

codeclimate 4 issues to fix
Details
WIP Ready for review
Details
codeclimate/diff-coverage 100% (99% threshold)
Details
codeclimate/total-coverage 96%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@solnic solnic deleted the type-specs-with-output-rules branch Jan 24, 2019

@timriley

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

@solnic I wonder if we should update dry-logic's type predicates to fully align with dry-type's type names, e.g. int? => integer?

@solnic

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

@timriley hah funny you mentioned it because it crossed my mind yesterday when I was updating :int => :integer 😄 yeah we could rename them, esp that we'll be inferring type predicates from type specs, so people won't have to use them explicitly.

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