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

ActionController::Parameters#require signature needs adjustment #231

Open
andyw8 opened this issue Mar 5, 2024 · 5 comments
Open

ActionController::Parameters#require signature needs adjustment #231

andyw8 opened this issue Mar 5, 2024 · 5 comments

Comments

@andyw8
Copy link
Contributor

andyw8 commented Mar 5, 2024

See #224 (review)

@amomchilov amomchilov changed the title ActionController::Parameters#require signature needs adjusted ActionController::Parameters#require signature needs adjustment Mar 5, 2024
@aprescott
Copy link

aprescott commented Aug 1, 2024

Just ran into this same issue, where params.require(:foo) is a string, integer, an array, etc.

I can T.cast(params.require(:foo), ...) although interestingly it doesn't always fire runtime errors, as in cases like a T::Array[X] vs. T::Array[Y].

@andyw8
Copy link
Contributor Author

andyw8 commented Aug 1, 2024

cc @amomchilov

@amomchilov
Copy link
Contributor

@aprescott Do you know what the full possible set of returnable types is?

It should be T.any(String, Integer, T::Array[T.any(String, Integer)) at a minimum, but do you know of any other possible cases?

Whatever that type is, that's what should be used as the return value of not just #require, but also #[], which still returns T.untyped

although interestingly it doesn't always fire runtime errors, as in cases like a T::Array[X] vs. T::Array[Y].

That's a performance concession, Sorbet's runtime type-checking doesn't do deep recursive type-checks on Enumerables. The runtime type-checking calls #valid?, not #recursively_valid?

@aprescott
Copy link

Do you know what the full possible set of returnable types is?

I'm not sure! It would be good to match whatever Parameters can possibly support. String or Integer or Array containers of those certainly sounds reasonable. Perhaps Rails source would help confirm. I don't really know much about the rbi-central project's approach here, but perhaps T.anything is an option if the full set of output types isn't clear? The true correct typing would of course beat T.anything.

Sorbet's runtime type-checking doesn't do deep recursive type-checks on Enumerables

Found the docs explaining this right after my comment, realized my confusion!

@amomchilov
Copy link
Contributor

String or Integer or Array containers of those certainly sounds reasonable.

Agreed, let's start there, and adjust it if people find that other values were possible.

@aprescott Would you be interested in opening your first RBI Central PR to contribute this Sig? :)

I don't really know much about the rbi-central project's approach here, but perhaps T.anything is an option if the full set of output types isn't clear?

I like using T.anything in my own code, but it's not a good fit for RBI central, because:

  1. It gets in peoples' way. Personally I find that to be a good thing, to make me think about the types and act accordingly. But others see it as an intrusive hassle. Ruby developers are already used to dealing with unknown types, anyway.
  2. It's kicking the can down the road. RBI Central is supposed to do the homework to provide the types, and T.anything largely indicates "🤷 we don't even know, either"

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

No branches or pull requests

3 participants