-
Notifications
You must be signed in to change notification settings - Fork 122
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 compiler for ActiveRecord delegated types #1241
Conversation
954c50e
to
6b37b76
Compare
Any more feedback @KaanOzkan ? |
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.
It LGTM but I'd like another review from someone with experience on delegated types.
Also can we fix the linting error? You can rebase to fix rest of the CI.
62c2c4a
to
a20d94c
Compare
Thanks - I have fixed that white space error and rebased |
mod.create_method( | ||
"#{role}_name", | ||
parameters: [], | ||
return_type: "String", |
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.
String isn't the return type of role_name
. It is a ActiveSupport::StringInquirer
.
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.
👍
) | ||
|
||
mod.create_method( | ||
"build_#{role}", |
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.
Ins't build_#{role}
already added by the belongs_to
DSL generator?
"build_#{association_name}", |
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 don’t think so because the belongs_to
is polymorphic
Could you have another look @rafaelfranca ? |
ActiveRecord doesn't store any metadata about delegated types, so we need to track this ourselves
Those ci failures against 6.1 are odd :
doesn't seem related to my changes. It also fails on master for me when I run that test file on its own |
Hey @fcheung, the build has been fixed and once you rebased |
db1a356
to
216f6e6
Compare
Thanks @st0012 - I have rebased |
This is also looking like an unrelated failure
|
I checked out this branch on our app, the types look correct and |
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.
Thanks for the PR, this looks great!
describe "with relations enabled" do | ||
before do | ||
require "tapioca/dsl/compilers/active_record_relations" | ||
activate_other_dsl_compilers(ActiveRecordRelations) | ||
end | ||
|
||
describe "without errors" do |
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 think we don't strictly need either of these describe
blocks, since they don't seem to be used in any of the test cases below. I know this PR has been open for a long time, so I'm more than happy to refactor this after a merge.
Motivation
ActiveRecord's delegated types feature adds a number of methods to active record models that were not included in the rbis generated by tapioca (See also #1240)
Implementation
Unlike associations, scopes etc. activerecord does not keep any internal metadata about which delegated types have been declared. I added an extension that records the parameters that
delegated_type
was called with so that this data can later be accessed from the compilerTests
Tests added to spec/tapioca/dsl/compilers/active_record_delegated_types_spec.rb