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

Pass an option to ActiveRecordAssociations compiler for how to generate association types #1993

Conversation

stathis-alexander
Copy link
Contributor

@stathis-alexander stathis-alexander commented Aug 18, 2024

Motivation

With the introduction of the column type option for the ActiveRecordColumn, we are finally almost able to migrate from the now deprecated Sorbet Rails onto Tapioca for our AR model class RBIs. However, the option only affects active record columns that have a non-null DB index. (A separate related issue is that these should respect validations as well; addressed in #1996).

Another common, related pattern that makes Tapioca difficult to adopt is that has_one or belongs_to associations can also be declared as non-nil through the required and optional kwargs, respectively. Presumably the justification for this behavior is identical to that of the active record columns; i.e., newly instantiated models may not have these relationships setup yes, despite the standard case being that the model is actually persisted and fetched from the DB.

This PR introduces a new ActiveRecordAssociationTypes option with possible values nilable and persisted. The default is nilable and retains the existing, backwards compatible behavior of always generating nil-able types for associations.

With the persisted option set, a belongs_to relationship would generate:

belongs_to :widget, class_name: 'Widget'
# sig { returns(::Widget) }
# def widget; end

belongs_to :doohickey, class_name: 'Doohickey', optional: true
# sig { returns(T.nilable(::Doohickey))
# def doohickey; end

Implementation

We follow the implementation in #1888 to add a new compiler option.

Tests

Tests have been added to address any basic cases.

@stathis-alexander
Copy link
Contributor Author

cc: @paracycle who implemented the original behavior.

@stathis-alexander stathis-alexander force-pushed the ajs/column-type-options-associations branch 2 times, most recently from c895a67 to 29639e1 Compare August 19, 2024 14:10
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

I think this is fine to go ahead with the implementation. I don't have context on the reflection options but it looks normal.

lib/tapioca/dsl/compilers/active_record_associations.rb Outdated Show resolved Hide resolved
lib/tapioca/dsl/compilers/active_record_associations.rb Outdated Show resolved Hide resolved
@stathis-alexander
Copy link
Contributor Author

@KaanOzkan Thank you for the comments! I have rewritten this for clarity, splitting the type option out into its own proper option. I have chosen a default of "nilable", since this retains the existing default behavior.

reflection.options[:required]
end

# Note - one can do more here. If the FK defining the belongs_to association is non-nullable at the DB level, or
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt it best to introduce this and then iterate later, rather than try to do to much at once. I left these notes for posterity, but I can remove them if it's unwanted.

If we do improve / extend the behavior here, we will likely want to dry up the code with that in the AR columns compiler.

@stathis-alexander stathis-alexander changed the title switch association nil behavior on new column type config option define association type option and generate non-nilable sigs when set to persisted Aug 21, 2024
@KaanOzkan KaanOzkan added the enhancement New feature or request label Aug 26, 2024
@KaanOzkan KaanOzkan changed the title define association type option and generate non-nilable sigs when set to persisted Pass an option to ActiveRecordAssociations compiler for how to generate association types Aug 27, 2024
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

This LGTM (I changed the title) but we need compiler documentation similar to 4c458b6. You can update the comment in the compiler file and then run bin/docs to update the corresponding markdown file.

You could also mention this compiler option in the relevant README section but it's not necessary.

@stathis-alexander stathis-alexander force-pushed the ajs/column-type-options-associations branch from 6cec578 to c03ac9f Compare September 1, 2024 14:09
@stathis-alexander stathis-alexander force-pushed the ajs/column-type-options-associations branch 2 times, most recently from 34ec644 to b01568b Compare September 1, 2024 14:21
@stathis-alexander
Copy link
Contributor Author

@KaanOzkan , apologies for the delay here. I was traveling last week. I have updated the documentation. I believe this is ready for release.

@stathis-alexander stathis-alexander force-pushed the ajs/column-type-options-associations branch from b01568b to c2ee70b Compare September 5, 2024 00:06
@KaanOzkan
Copy link
Contributor

Opened a PR to fix CI #2012

@stathis-alexander
Copy link
Contributor Author

Hello @paracycle ! Thank you so much for your careful review and comments. I believe I have addressed them all.

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Thanks for this, looks great to me!

@egiurleo
Copy link
Contributor

Hi, @stathis-alexander, we just fixed CI on Ruby 3.3. Would you mind rebasing on main and force pushing your branch?

@stathis-alexander
Copy link
Contributor Author

Hi @egiurleo , I was under the impression based on the conversation here that this pull request would not be accepted.

@egiurleo
Copy link
Contributor

Oh I apologize, I hadn't seen that other conversation. Let me catch up on it now.

@stathis-alexander
Copy link
Contributor Author

Closing due to the conversation here. AL is using https://github.com/angellist/boba now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants