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 validators module to all validators #2200

Merged
merged 4 commits into from
Dec 5, 2021

Conversation

ericproulx
Copy link
Contributor

Hello,

While looking at the "validations/validators" folder, I found that 'validators" is not a declared module and I was wondering why. I looked at the "validations/types" and "types" is a declared module. Anyway, I thought that for consistency, it had to be done.

There were also a lot of LeakyConstantDeclaration inside validators specs. I figured it would be better to use AnonymousClasses instead. Unfortunately, declaring an anonymous Grape::API for every single test was slowing down everything and I thought using let_it_be from test-prof would be ideal.

I think rubocop-rspec could be a great addition for the spec. I could open a PR if you want.

@dblock
Copy link
Member

dblock commented Dec 5, 2021

Good stuff.

+1 on rubocop-rspec, please

@dm1try
Copy link
Member

dm1try commented Dec 31, 2021

hmm, it's a breaking change for custom validators so UPGRADING.md should be updated.

jrmhaig added a commit to ministryofjustice/Claim-for-Crown-Court-Defence that referenced this pull request Jan 12, 2022
ruby-grape/grape#2200 moves Grape::Validations::Base to
Grape::Validations::Validators::Base and this is included in the 1.6.1 release
of the grape gem.
dm1try added a commit to dm1try/grape that referenced this pull request Feb 17, 2022
@dblock
Copy link
Member

dblock commented Feb 19, 2022

hmm, it's a breaking change for custom validators so UPGRADING.md should be updated.

Lessons learned here, we should have made a major version bump or built backwards compat, see #2244

dm1try added a commit to dm1try/grape that referenced this pull request Feb 19, 2022
@ericproulx ericproulx mentioned this pull request Nov 13, 2023
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.

3 participants