sorbet-rails icon indicating copy to clipboard operation
sorbet-rails copied to clipboard

Use HashWithIndifferentAccess for enum values

Open hdoan741 opened this issue 5 years ago • 5 comments

Currently sorbet-rails uses T::Hash[T.any(String, Symbol), Integer] for enum values of a model. This works but the keys method would return an array of Symbol or String. Rails actually use HashWithIndifferentAccess, which guarantee (softly) that .keys would return an array of String.

It's blocking on the typedef of HashWithIndifferentAccess itself (which should/could be added to sorbet-typed)

hdoan741 avatar Jun 21 '19 18:06 hdoan741

Just curious - is this something we still want to resolve? It seems like ActiveSupport::HashWithIndifferentAccess now exists in sorbet-typed.

If so, happy to submit a PR!

mattxwang avatar Jul 01 '22 18:07 mattxwang

If so, happy to submit a PR!

Yes please!

ghiculescu avatar Jul 01 '22 18:07 ghiculescu

This is my first time contributing to sorbet-rails, so forgive me for my lack of experience; I have a few questions!

My understanding is that to change the enum values for a model, I should look to change the types in active_record_enum.rb. Is that correct?

And to add - looking at the class def for ActiveSupport::HashWithIndifferentAccess, it subclasses their Hash type, which I'm not sure if it's a template type. Am I understanding how to use this properly?

i.e., should my new code look like this?

return_type = if value_type.length == 1
  "ActiveSupport::HashWithIndifferentAccess[#{value_type.first}]" # should this template type be here?
else
  "ActiveSupport::HashWithIndifferentAccess[T.any(#{value_type.join(', ')})]" # should this template type be here?
end

Or this; but, I feel like I'm losing some type information?

return_type = "ActiveSupport::HashWithIndifferentAccess"

mattxwang avatar Jul 01 '22 20:07 mattxwang

Hmm, this works fine, but as you say it loses type information.

But this does not.

@jeffcarbs you added the ActiveSupport::HashWithIndifferentAccess sig here but do you mind sharing how it's typically used?

ghiculescu avatar Jul 03 '22 16:07 ghiculescu

While we wait for @jeffcarbs' answer, I'll note that the documentation for the class has examples where values are of different types:

rgb = ActiveSupport::HashWithIndifferentAccess.new

rgb[:black] = '#000000'
hash = ActiveSupport::HashWithIndifferentAccess.new(a: 1)

So I'm sure that there are typical uses where the values are different types.

This seems suitable for a generic type to me, which may make upgrading for users easier since it then just becomes a find-and-replace for T::Hash[T.any(String, Symbol), -> ActiveSupport::HashWithIndifferentAccess[. In that case, we also won't lose type information!

However, I'm not sure if this is bad practice since we're modifying how ActiveSupport::HashWithIndifferentAccess is defined internally, and that sounds like a breaking change to me. It also seems like generics are still an experimental feature in sorbet. My understanding is that we would lose generic types (type erasure?). It may be difficult to implement (though I'd still like to try if possible!).

Any thoughts @ghiculescu? I'm coming with little experience with sorbet or rails (my background is not in ruby) so any thoughts are appreciated!

mattxwang avatar Jul 12 '22 22:07 mattxwang