-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add the contract
DSL
#2419
Add the contract
DSL
#2419
Conversation
Resolves ruby-grape#2386
There's also the question of whether |
@dblock @dnesteryuk @zysend @Jack12816 Tagging a few of the recent contributors, as suggested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty great! See some comments, mostly small.
Also added support for This seems like the price to pay for having |
And change the nesting
See the discussion in ruby-grape#2419
And add a test for that. ruby-grape#2386
c784875
to
79f37c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like workflow is borked, https://github.com/ruby-grape/grape/actions/runs/8383399387. Plus some nitpicks.
Danger still complains about the TOC, which needs to be fixed.
Looks great otherwise!
I've added a TOC entry yesterday, and it works: https://github.com/dgutov/grape/tree/contract_dsl_for_dry_schema?tab=readme-ov-file#using-dry-validation-or-dry-schema Perhaps the checker is thrown off by backtick formatting in the text? Is there a way to see its output somewhere? The workflow just says "Danger has failed this build". And when I try to run it locally, I get various errors, such as
or
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix danger complaining unquote the text in the TOC.
- [Using dry-validation or dry-schema](#using-dry-validation-or-dry-schema)
(danger comments in the PR with the expected TOC, see above)
Done.
Ok I see: it edits the original comment. Neat. |
Finally, I've added Rubocop exceptions. Those remaining warnings didn't look reasonable to me personally. If you agree, I can try a separate PR that disables the |
That's fine. Usually I rubocop for auto formatting and consistency. You can always |
Merged this, good work! |
Hi!
This seems to cover most or everything we've discussed in the issue comments (except for "rules", as expected). It turned out pretty compact.
Not many docs yet. Comments about naming or organization welcome.
I've thought about moving the new
Validator
to a separate file, which would allow testing it in isolation, but then there wouldn't be much left inContractScope
, and all the validators are tested integration-style anyway.From
Validator
's constructor arguments, we can see that this stretches the currentValidatorFactory
protocol, but I figured that it's not worth a major change just for this.Also: I wasn't sure what would be the correct signature for the
klass
parameter in the doc, hence this syntax. Maybe it'd be better to leave the type not annotated.Resolves #2386