Skip to content

Add the contract DSL - #2419

Merged
dblock merged 23 commits into
ruby-grape:masterfrom
dgutov:contract_dsl_for_dry_schema
Mar 24, 2024
Merged

dblock merged 23 commits into
ruby-grape:masterfrom
dgutov:contract_dsl_for_dry_schema

Conversation

@dgutov

@dgutov dgutov commented Mar 21, 2024

Copy link
Copy Markdown
Contributor

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 in ContractScope, and all the validators are tested integration-style anyway.

From Validator's constructor arguments, we can see that this stretches the current ValidatorFactory 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

@dgutov

dgutov commented Mar 21, 2024

Copy link
Copy Markdown
Contributor Author

There's also the question of whether dry-schema should become a hard dependency of this gem, or whether ContractScope.new should just raise an error when Dry::Schema is not defined.

@dgutov

dgutov commented Mar 21, 2024

Copy link
Copy Markdown
Contributor Author

@dblock @dnesteryuk @zysend @Jack12816 Tagging a few of the recent contributors, as suggested.

@dblock dblock left a comment

Copy link
Copy Markdown
Member

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.

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread lib/grape/dsl/validations.rb Outdated
Comment thread spec/grape/validations/contract_scope_spec.rb
Comment thread README.md
@dgutov

dgutov commented Mar 21, 2024

Copy link
Copy Markdown
Contributor Author

Also added support for declared just now. It re-processes the set of keys, but stops from repeating the same validations, fortunately.

This seems like the price to pay for having declared work having combined params and contract validations work naturally.

@dgutov
dgutov force-pushed the contract_dsl_for_dry_schema branch from c784875 to 79f37c3 Compare March 21, 2024 22:28

@dblock dblock left a comment •

Copy link
Copy Markdown
Member

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!

Comment thread .github/workflows/test.yml
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread lib/grape/dsl/validations.rb
@dgutov

dgutov commented Mar 22, 2024

Copy link
Copy Markdown
Contributor Author

Danger still complains about the TOC, which needs to be fixed.

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

fatal: ambiguous argument 'origin/master': unknown revision or path not in the working tree.

or

.../gems/danger-9.4.3/lib/danger/request_sources/github/github.rb:117:in `fetch_details': undefined method `pull_request' for nil:NilClass (NoMethodError)

@dblock dblock left a comment •

Copy link
Copy Markdown
Member

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)

Comment thread .github/workflows/test.yml
@dgutov

dgutov commented Mar 22, 2024

Copy link
Copy Markdown
Contributor Author

To fix danger complaining unquote the text in the TOC.

- [Using dry-validation or dry-schema](#using-dry-validation-or-dry-schema)

Done.

(danger comments in the PR with the expected TOC, see above)

Ok I see: it edits the original comment. Neat.

@dgutov

dgutov commented Mar 22, 2024

Copy link
Copy Markdown
Contributor Author

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 RSpec/FilePath and RSpec/SpecFilePathFormat at least for all specs in the integration dir, and either disables the RSpec/MultipleExpectations cop altogether, or at least raises the limit.

@dblock

dblock commented Mar 24, 2024

Copy link
Copy Markdown
Member

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 RSpec/FilePath and RSpec/SpecFilePathFormat at least for all specs in the integration dir, and either disables the RSpec/MultipleExpectations cop altogether, or at least raises the limit.

That's fine. Usually I rubocop for auto formatting and consistency. You can always rubocop -a ; rubocop --auto-gen-config.

@dblock
dblock merged commit 7ec1f51 into ruby-grape:master Mar 24, 2024
@dblock

dblock commented Mar 24, 2024

Copy link
Copy Markdown
Member

Merged this, good work!

@dgutov
dgutov deleted the contract_dsl_for_dry_schema branch March 25, 2024 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Define contract instead of params

2 participants