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

feat(core): add isPromise generic #34168

Closed
wants to merge 1 commit into from
Closed

feat(core): add isPromise generic #34168

wants to merge 1 commit into from

Conversation

danranVm
Copy link
Contributor

@danranVm danranVm commented Dec 1, 2019

add generic to help type inference

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@danranVm danranVm requested review from a team as code owners December 1, 2019 09:18
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

I would squash the three commits into one and use the more general refactor: make addPromise() generic commit message.

Otherwise LGTM

@petebacondarwin petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: presubmit The PR is in need of a google3 presubmit refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release labels Dec 2, 2019
@ngbot ngbot bot added this to the needsTriage milestone Dec 3, 2019
@mhevery mhevery added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: presubmit The PR is in need of a google3 presubmit labels Mar 6, 2020
@mhevery
Copy link
Contributor

mhevery commented Mar 6, 2020

presubmit

@mhevery mhevery self-assigned this Mar 6, 2020
@matsko
Copy link
Contributor

matsko commented Mar 9, 2020

@matsko
Copy link
Contributor

matsko commented Mar 11, 2020

@danranVm please rebase so we can see if this works with TypeScript 3.8.

@AndrewKushnir AndrewKushnir added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 11, 2020
@IgorMinar IgorMinar added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Mar 11, 2020
@IgorMinar
Copy link
Contributor

fyi: I just relabeled this PR as "master-only" since it's a "feature" that does not belong on the patch branch.

@AndrewKushnir AndrewKushnir removed the action: merge The PR is ready for merge by the caretaker label Mar 12, 2020
@danranVm
Copy link
Contributor Author

@danranVm please rebase so we can see if this works with TypeScript 3.8.

@matsko done

@AndrewKushnir
Copy link
Contributor

Hi @danranVm, thanks for rebasing this PR. I've noticed that there is a new comment that was added as a result of rebase (refactor: rebase code). Could you please squash commits, so there is just the first one? Thank you.

@danranVm
Copy link
Contributor Author

@AndrewKushnir ok, it's correct now ?

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 18, 2020
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Mar 18, 2020

Hi @danranVm, thanks for updating commit messages, everything looks good now. I've started a new g3 presubmit (internal only link) and will keep you updated. Thank you.

@AndrewKushnir AndrewKushnir added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 18, 2020
@AndrewKushnir
Copy link
Contributor

@danranVm, sorry, just noticed that the commit message refers to addPromise function, but the actual function being updated in this PR is isPromise. Also, the commit description uses refactor: namespace, when PR title says feat(core). I'd propose to update commit message to something like this (also include why this change is made):

feat(core): add `isPromise` generic

This commit adds generic to `isPromise` function to help with type inference.

Could you please update commit message?

Thank you.

This commit adds generic to `isPromise` function to help with type inference.
@danranVm
Copy link
Contributor Author

@AndrewKushnir updated, thanks for your review.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: presubmit The PR is in need of a google3 presubmit target: major This PR is targeted for the next major release labels Mar 19, 2020
@AndrewKushnir
Copy link
Contributor

Hi @danranVm, thanks for the updates. The commit message looks good and g3 presubmit is successful, so I'm adding the "merge" label back. I also updated the "PR target" to be master and patch, since we cut 9.1.0 RC.0 this morning and created a new patch branch for 9.1.x versions. Thank you.

@mhevery mhevery closed this in 47bfec4 Mar 20, 2020
@danranVm danranVm deleted the dev-ispromise branch April 19, 2020 08:21
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: zones cla: yes refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants