8000 Add types to `T::Struct.new` by azdavis · Pull Request #986 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add types to T::Struct.new #986

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 26 commits into from
Jun 27, 2019
Merged

Add types to T::Struct.new #986

merged 26 commits into from
Jun 27, 2019

Conversation

azdavis
Copy link
Contributor
@azdavis azdavis commented Jun 21, 2019 •
edited
Loading

This makes subclasses of T::Struct have good static types for new.

I would recommend reviewing by commit.

Motivation

To increase typedness.

Test plan

See included automated tests.

@azdavis azdavis requested review from jez and a team June 21, 2019 20:30
@ghost ghost removed their request for review June 21, 2019 20:30
azdavis
azdavis commented Jun 21, 2019
View reviewed changes
@azdavis
Copy link
Contributor Author
azdavis commented Jun 21, 2019

Oh, I just remembered I need to check whether the superclass in the ClassDef is T::Struct. If not, we don't want to synthesize an initialize.

@DarkDimius
Copy link
Collaborator

This seems to refactor a lot of code additionally to actual changes in behavior.
Can we land prework of refactors(that are not supposed to change behavior) as a separate PR?

@azdavis azdavis force-pushed the azdavis/add-types-to-struct-initialize branch from ee4460f to cebb340 Compare June 21, 2019 21:35
@azdavis
Copy link
Contributor Author
azdavis commented Jun 21, 2019

Sure. What were you thinking could be landed as pre-work? Maybe everything except the logic that actually creates the synthesized initialize method (i.e., everything except the block inside synthesizeInitialize) and the tests?

@azdavis azdavis force-pushed the azdavis/add-types-to-struct-initialize branch 3 times, most recently from bf71a19 to 3bcd365 Compare June 22, 2019 00:01
@DarkDimius
Copy link
Collaborator

Maybe everything except the logic that actually creates the synthesized initialize method (i.e., everything except the block inside synthesizeInitialize) and the tests?

that would be perfect!

@azdavis azdavis mentioned this pull request Jun 24, 2019
Merged
@azdavis azdavis force-pushed the azdavis/add-types-to-struct-initialize branch from 3bcd365 to 246c9f3 Compare June 24, 2019 17:56
@azdavis
Copy link
Contributor Author
azdavis commented Jun 24, 2019

@jez Bumping this now that #1024 has been merged.

jez
jez reviewed Jun 24, 2019
View reviewed changes
jez
jez reviewed Jun 24, 2019
View reviewed changes
@azdavis azdavis mentioned this pull request Jun 24, 2019
Merged
@azdavis azdavis force-pushed the azdavis/add-types-to-struct-initialize branch 4 times, most recently from 332c5b2 to c7dba53 Compare June 25, 2019 00:26
@azdavis azdavis mentioned this pull request Jun 25, 2019
Merged
@azdavis azdavis force-pushed the azdavis/add-types-to-struct-initialize branch 4 times, most recently from 1412388 to 77935fa Compare June 26, 2019 22:39
@azdavis
Copy link
Contributor Author
azdavis commented Jun 27, 2019

@jez bumping now that the last pay-server pr is in master-passing-tests.

@azdavis azdavis requested a review from jez June 27, 2019 17:07
@azdavis azdavis force-pushed the azdavis/add-types-to-struct-initialize branch from 77935fa to 093be99 Compare June 27, 2019 17:16
@azdavis azdavis force-pushed the azdavis/add-types-to-struct-initialize branch from ce29d3b to faa3e2a Compare June 27, 2019 22:01
@azdavis azdavis requested a review from jez June 27, 2019 22:02
jez
jez reviewed Jun 27, 2019
View reviewed changes
jez
jez approved these changes Jun 27, 2019
View reviewed changes
Copy link
Collaborator
@jez jez left a comment

Choose a reason for hiding this comment

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

Other than the Inexact.new error w.r.t. RBIs, this looks great! Thanks!

@azdavis azdavis merged commit c293b55 into master Jun 27, 2019
@azdavis azdavis deleted the azdavis/add-types-to-struct-initialize branch June 27, 2019 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jez jez jez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@azdavis @DarkDimius @jez
0