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

Annotate ActionController::Parameters#require #224

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

bdewater
Copy link
Contributor

Type of Change

This can now be typed properly with overloaded signatures.

  • Add RBI for a new gem
  • Modify RBI for an existing gem
  • Other:

Changes

@bdewater bdewater requested a review from a team as a code owner February 26, 2024 14:51
@KaanOzkan KaanOzkan merged commit cdf6f7b into Shopify:main Feb 26, 2024
3 checks passed
@bdewater bdewater deleted the actionpack-params-require branch February 26, 2024 21:58
@Morriar Morriar added the rbi Change related to RBI annotations label Mar 1, 2024
@amomchilov amomchilov changed the title Annotate ActionController::Parameters#require Annotate ActionController::Parameters#require Mar 5, 2024
Comment on lines +114 to +115
sig { params(key: T.any(String, Symbol)).returns(ActionController::Parameters) }
sig { params(key: T::Array[T.any(String, Symbol)]).returns(T::Array[ActionController::Parameters]) }
Copy link
Contributor

@amomchilov amomchilov Mar 5, 2024

Choose a reason for hiding this comment

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

I don't think these are correct, because require can return "scalar" values:

params = ActionController::Parameters.new(composite: { a: 1, b: 2 }, scalar: 123)
composite_value, scalar_value = params.require([:composite, :scalar])

p composite_value # ✅ => #<ActionController::Parameters {"a"=>1, "b"=>2} permitted: false>
p scalar_value # ❌ => 123

Tracked in: #231

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rbi Change related to RBI annotations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants