Skip to content

feat(40750): Refactoring to add inferred return type annotation to a function #41052

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 1 commit into from
Nov 4, 2020

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #40750

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 12, 2020
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good. I think this is a useful refactoring. There is a more general, less useful refactor that adds a type annotation on any declaration that could have one, but doesn't need to. I don't think this PR should add that (1) because it's less useful (2) I can't remember how to make a single refactoring produce multiple fixes -- it may not be possible.

@jessetrinity can you take a look too? What is your opinion on the more general refactoring?

@sandersn
Copy link
Member

@gabritto you might have opinions on the question of the more general refactoring.

@sandersn
Copy link
Member

Sounds like nobody else wants to chime in, so I'll merge this after the 4.1 RC goes out.

@jessetrinity jessetrinity self-assigned this Oct 28, 2020
Copy link
Contributor

@jessetrinity jessetrinity left a comment

Choose a reason for hiding this comment

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

Perhaps there should be some tests with multiple signatures:

// bad code - there should still be a test so that we at least know what the refactor does in this case, if doing nothing is too complicated.
function f(x: number) {
    return 1;
}
function f(x: number) {
    return "1";
}
// good code (?) - grabbing the first signature will add "number" as the return type, but we probably wanted "number | string"
function f(x: number): number;
function f(x: string): string;
function f(x: string | number) {
  return typeof(x) === "string" ? "a" : 1;
}

@a-tarasyuk a-tarasyuk force-pushed the feat/40750 branch 2 times, most recently from 6882451 to e021e67 Compare November 3, 2020 19:07
@sandersn sandersn merged commit 0904865 into microsoft:master Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Refactoring to add inferred return type annotation to a function
4 participants