8000 drop extraneous checks in `T::Types::ClassOf#valid?` by froydnj · Pull Request #8797 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

drop extraneous checks in T::Types::ClassOf#valid? #8797

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

Merged
merged 2 commits into from
Apr 25, 2025

Conversation

froydnj
Copy link
Contributor
@froydnj froydnj commented Apr 25, 2025

Motivation

We don't need the extra checking for obj.is_a?(Module) because we're never doing anything with obj that depends on Module-ness. (We do have test for non-Module objs, so if tests pass, this change should be fine.)

#6810 is defunct now, I think, but it had a test that seems reasonable to import.

Test plan

Existing tests.

@froydnj froydnj requested a review from a team as a code owner April 25, 2025 13:40
@froydnj froydnj requested review from neilparikh and removed request for a team April 25, 2025 13:40
@froydnj
Copy link
Contributor Author
froydnj commented Apr 25, 2025

We have a policy of testing changes to Sorbet against Stripe's codebase before
merging them. I've kicked off a test run for the current PR. When the build
finishes, I'll share with you whether or how it failed. Thanks!

Stripe employees can see the build results here:

https://go/builds/bui_SCBtVqvpALiKbN
https://go/builds/bui_SCBttTu4Axy9gD
https://go/builds/bui_SCBt2MpKnwHCxQ

This change also has sorbet-runtime changes. Those tests:

https://go/builds/bui_SCC0orJPdfb5Tx

@froydnj froydnj merged commit b56c299 into master Apr 25, 2025
15 checks passed
@froydnj froydnj deleted the froydnj-drop-module-class-of-check branch April 25, 2025 15:14
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

Successfully merging this pull request may close these issues.

2 participants
0