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

Provide a hook for custom ActiveModel types #2112

Merged

Conversation

rzane
Copy link
Contributor

@rzane rzane commented Dec 5, 2024

Motivation

We use ActiveModel::Attributes extensively. In many cases, we're passing complex values to these models, such as a User. It wouldn't be practical in our situation to define an ActiveModel::Type class for each and every possible value that gets passed into a model.

Any of the following solutions would solve my issue:

  1. Explicitly tell Tapioca the type of a given attribute.
  2. Explicitly tell Tapioca to skip type generation for a given attribute, which would allow me to write my own compiler for these specific attributes.
  3. Allow compilers to override the types defined by other compilers (similar to Add explicit ability to choose order of Tapioca compiler execution #1965).

Implementation

This PR allows instances of ActiveModel::Type::Value to define an optional method #__tapioca_type. It returns a type.

For example, you could use this hook to implement an interface like this:

class GenericType < ActiveModel::Type::Value
  def initialize(type)
    @type = type
  end

  def __tapioca_type = @type
end

class User
end

class Foo
  include ActiveModel::Attributes

  attribute :user, GenericType.new(User)
end

Tests

Tests are included.

@rzane rzane requested a review from a team as a code owner December 5, 2024 01:48
@rzane rzane force-pushed the hooks-for-custom-active-model-attributes branch from aca3e3b to d103457 Compare December 5, 2024 13:17
@paracycle
Copy link
Member

It wouldn't be practical in our situation to define an ActiveModel::Type class for each and every possible value that gets passed into a model.

You can define a "generic" ActiveModel::Type subclass and use it to supply such a hook, provided we add support for it in Tapioca.

For example:

class GenericType < ActiveModel::Type::Value
  def initialize(type)
    @type = type
  end

  def __sorbet_type = @type
end

class User
end

class Foo
  include ActiveModel::Attributes

  attribute :foo, GenericType.new(User)
end

p Foo.attribute_types.first.last.__sorbet_type # => User

@rzane
Copy link
Contributor Author

rzane commented Dec 9, 2024

Thanks for the suggestion @paracycle. I'll update this PR to work that way.

@rzane rzane force-pushed the hooks-for-custom-active-model-attributes branch from d103457 to 158c705 Compare December 9, 2024 18:10
@rzane rzane requested a review from KaanOzkan December 9, 2024 18:12
@rzane
Copy link
Contributor Author

rzane commented Dec 9, 2024

PR updated. Thank you for the suggestion. I think this is a much better solution.

@amomchilov amomchilov changed the title Provide a hook for custom activemodel types Provide a hook for custom ActiveModel types Dec 10, 2024
@amomchilov amomchilov added the enhancement New feature or request label Dec 10, 2024
Copy link
Contributor

@amomchilov amomchilov left a comment

Choose a reason for hiding this comment

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

Hey @rzane, we like this change and want to merge it in. We just got a bit of internal feedback, that this method is really more for Tapioca, and not Sorbet itself.

__type_for_tapioca and __tapica_type was suggested, which would make that more clear, and better help readers understand where to look for callers of this (seemingly unused) method.

Would you mind renaming the method to one of those?

@rzane
Copy link
Contributor Author

rzane commented Dec 11, 2024

Thanks @amomchilov, updated 👍

@rzane rzane force-pushed the hooks-for-custom-active-model-attributes branch from 3760515 to e198957 Compare December 11, 2024 01:54
@rzane rzane force-pushed the hooks-for-custom-active-model-attributes branch from e198957 to e6839dc Compare December 11, 2024 01:55
@rzane
Copy link
Contributor Author

rzane commented Dec 11, 2024

I just want to call out that there might be an existing convention for this sort of thing:

sorbet_type = if type.respond_to?(:sorbet_type)
type.sorbet_type

The type isn't used by Sorbet directly, but it does have to be a type that is compatible with Sorbet and will eventually be consumed by Sorbet after generating DSL files.

@rzane
Copy link
Contributor Author

rzane commented Dec 12, 2024

@amomchilov are there any remaining blockers here?

@amomchilov
Copy link
Contributor

amomchilov commented Dec 12, 2024

Nope, looks good! Thanks for the changes.

I'm deprecating #sorbet_type and renaming it to match, in #2125.

@amomchilov amomchilov merged commit 7ff5fcd into Shopify:main Dec 12, 2024
15 checks passed
@rzane rzane deleted the hooks-for-custom-active-model-attributes branch December 13, 2024 01:06
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