Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 contract support to actions #453
Add contract support to actions #453
Changes from all commits
e30f71a
42a76dd
37dbc3e
e63ba72
408524d
63338ab
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'm conflicted about this decision. The reason why I would most likely want to use a concrete class for this is to share validation rules between multiple things.
By basing this on
Hanami::Action::Params
, this requires a request env to initialize, which limits its usefulness to controllers only. This excludes non-request sharing, such as Operations or CLI tools.I wish contracts were composable. If I could merge a preexisting contract into the action validator, this would be moot.
Another way you can share logic is via macros. My preference would be to define an abstract class to contain these macros because I may want to use different ones for different use-cases. But since you hard-code
Dry::Validation::Contract
as the base class here, I can only share macros globally. I don't know if that's a problem.On the other hand, perhaps its reasonable that HTTP-boundary validations are their own thing.
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.
@alassek Thanks for raising this! I agree with you here. If I was building this feature from scratch today, I wouldn't have things run through these
Hanami::Action::Params
intermediary classes, and instead have dry-validation contracts be provided directly.I'd describe this current approach as a vestige of the 1.x-era of Hanami design, where it was much more heavy-handed in "owning the APIs" rather than the lighter touch approach we take for our integration with the dry-rb gems today. It's also worth mentioning that back in those times, dry-validation might have seemed like a slightly less stable target, so the framework builders might've wanted this extra layer as "insulation."
In terms of why I implemented it this way here: I wanted a small, minimally-disruptive change as a first step, something I could merge confidently and use as a launching point for any next steps. I didn't say it out loud, but I did want us to get to the point of supporting dry-validation contracts directly, but I wasn't sure about when would be ideal for that.
Your question motivates me to have a quick run at it as a follow-up and see what might be possible :)
As an additional measure, I'm going to mark the new
Hanami::API::Params.contract
method here as@api private
, as part of a "soft deprecation" approach to this direct use of the Params class. I'd much rather have users specify their validations at the action level only, and use contracts directly as a way to reuse it. Let's see how it goes!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.
I've started this work in #454. I think I have a pretty clear path to getting it done, too. I'll leave another update over in that PR once it's ready!
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.
#454 is ready now :)