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

Add the contract DSL #2419

Merged
merged 23 commits into from
Mar 24, 2024
Merged

Conversation

dgutov
Copy link
Contributor

@dgutov dgutov commented Mar 21, 2024

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
Copy link
Contributor Author

dgutov commented Mar 21, 2024

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
Copy link
Contributor Author

dgutov commented Mar 21, 2024

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

Copy link
Member

@dblock dblock left a 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.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/grape/dsl/validations.rb Outdated Show resolved Hide resolved
spec/grape/validations/contract_scope_spec.rb Show resolved Hide resolved
README.md Show resolved Hide resolved
@dgutov
Copy link
Contributor Author

dgutov commented Mar 21, 2024

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
Copy link
Member

@dblock dblock left a 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!

.github/workflows/test.yml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/grape/dsl/validations.rb Show resolved Hide resolved
@dgutov
Copy link
Contributor Author

dgutov commented Mar 22, 2024

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)

Copy link
Member

@dblock dblock left a 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)

.github/workflows/test.yml Show resolved Hide resolved
@dgutov
Copy link
Contributor Author

dgutov commented Mar 22, 2024

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
Copy link
Contributor Author

dgutov commented Mar 22, 2024

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
Copy link
Member

dblock commented Mar 24, 2024

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
37 checks passed
@dblock
Copy link
Member

dblock commented Mar 24, 2024

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