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

Implement ActiveModel::Validations::Confirm compiler #1644

Merged

Conversation

Tonkpils
Copy link
Contributor

@Tonkpils Tonkpils commented Sep 6, 2023

Motivation

Given a class implementing a confirmation validation from ActiveModel::Validations

class User < ApplicationRecord
  validates :password, confirmation: true
end

will generate virtual attributes password_confirmation and password_confirmation=. Without this compiler, usage of those methods will cause an error with Sorbet unless they are shimmed out.

Implementation

The compiler is pretty straightforward iterating over all validators of a gathered constant, selecting only those that implement ConfirmationValidator and generating the necessary methods for each of the attributes in the rbi file. It also skips ActiveRecord::Base since that class includes ActiveModel::Validations but does not implement any of its validations.

Tests

Tests were added

@Tonkpils Tonkpils requested a review from a team as a code owner September 6, 2023 22:11
@Tonkpils Tonkpils force-pushed the tonkpils/active-model-validations-confirm branch from 8df0b1c to 2e7702f Compare September 7, 2023 02:21
@Tonkpils Tonkpils force-pushed the tonkpils/active-model-validations-confirm branch from 2e7702f to 592b369 Compare September 7, 2023 15:54
# Create RBI definitions for all the attributes that use confirmation validation
confirmation_validators.each do |validator|
validator.attributes.each do |attr_name|
klass.create_method("#{attr_name}_confirmation", return_type: "T.untyped")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to figure out the right types based on the columns?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the it's a subclass of ActiveRecord::Base, we can probably use one of ActiveRecord's API to figure out the column type and translate it to a proper return type. The same could be said for classes that implement ActiveModel::Attributes.

However, since this validations can be added to POROs, we'd have to add logic to differentiate the two. Are we ok with doing that here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Do these validations require you to use Active Model attributes or something that would allow us to fetch the type?

If not, I think it's fine to use T.untyped. Just trying to think if there's a way to get better typing support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these validations require you to use Active Model attributes or something that would allow us to fetch the type?

Unfortunately no, they only require `ActiveModel::Validations

class Foo
   attr_reader :foo

   def initialize(foo)
     @foo = foo
   end

   include ActiveModel::Validations

   validates_confirmation_of :foo
end

f = Foo.new("foo")
f.foo_confirmation = "bar"
f.valid?
=> false
f.errors
=> #<ActiveModel::Errors [#<ActiveModel::Error attribute=foo_confirmation, type=confirmation, options={:attribute=>"Foo"}>]>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Yeah, it seems like this can be used in pretty much any context, let's go with T.untyped. Thanks for investigating!

# Create RBI definitions for all the attributes that use confirmation validation
confirmation_validators.each do |validator|
validator.attributes.each do |attr_name|
klass.create_method("#{attr_name}_confirmation", return_type: "T.untyped")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Yeah, it seems like this can be used in pretty much any context, let's go with T.untyped. Thanks for investigating!

@vinistock
Copy link
Member

It appears you need to run bin/docs to update the docs.

@Tonkpils
Copy link
Contributor Author

Tonkpils commented Sep 8, 2023

It appears you need to run bin/docs to update the docs.

All fixed 🙇

@vinistock
Copy link
Member

I think you might need to rebase the branch because of #1643. There are some typechecking and test errors.

@Tonkpils Tonkpils force-pushed the tonkpils/active-model-validations-confirm branch from 3ff555a to 71d0647 Compare September 10, 2023 02:46
@Tonkpils
Copy link
Contributor Author

All done @vinistock 🙇

@vinistock
Copy link
Member

It seems that typechecking is still failing.

@Tonkpils
Copy link
Contributor Author

@vinistock looks like this will be fixed by #1648?

@vinistock
Copy link
Member

Okay, rebasing now should fix the CI.

Tonkpils and others added 4 commits September 12, 2023 10:14
This validation generates a virtual attribute that allows for checking
confirmation logic over the given attribute name
@Tonkpils Tonkpils force-pushed the tonkpils/active-model-validations-confirm branch from 71d0647 to 4c49c4a Compare September 12, 2023 14:14
@Tonkpils
Copy link
Contributor Author

Done!

@vinistock
Copy link
Member

Thank you for the contribution!

@vinistock vinistock merged commit c129716 into Shopify:main Sep 12, 2023
15 checks passed
@shopify-shipit shopify-shipit bot temporarily deployed to production September 13, 2023 22:55 Inactive
@Tonkpils Tonkpils deleted the tonkpils/active-model-validations-confirm branch September 14, 2023 02:54
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.

2 participants