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

fix(common): add params and report progress to put #37873

Closed
wants to merge 1 commit into from

Conversation

ajitsinghkaler
Copy link
Contributor

When response type is Json the put sigbnature did not have report progress and params key. This makes it difiicult to type check added them to the signature

Fixes #23600

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: #23600

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: public-api

@ajitsinghkaler
Copy link
Contributor Author

Do we need a google3 presubmit on this??

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.

This seems like a perfectly reasonable fix unless someone can explain why this particular overload did not have these options? It looks like a simple oversight to me.

There are some typos in the commit message. Can you change it to:

fix(common): add `params` and `reportProgress` options to `HttpClient.put()` overload

When the response type is JSON, the `put()` overload signature did not have `reportProgress`
and  `params` options. This makes it difficult to type-check this overload.

This commit adds them to the overload signature.

Fixes #23600

@ajitsinghkaler
Copy link
Contributor Author

@petebacondarwin changed the message

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.

Reviewed-for: public-api

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.

Reviewed-for: public-api-approvers

@pullapprove pullapprove bot requested review from AndrewKushnir and atscott and removed request for IgorMinar July 23, 2020 03:33
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

Reviewed-for: fw-core

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@AndrewKushnir AndrewKushnir removed the request for review from pkozlowski-opensource July 28, 2020 18:36
@AndrewKushnir
Copy link
Contributor

FYI, the remaining group that needs to give an LGTM is fw-http (@alxhub).

@ajitsinghkaler
Copy link
Contributor Author

ajitsinghkaler commented Sep 15, 2020

@AndrewKushnir is there a problem with the presubmit. Should I change something in this.

@AndrewKushnir
Copy link
Contributor

Hi @ajitsinghkaler, I've started a new presubmit and will let you know how it goes. Meanwhile, could you please rebase this PR so we re-run CI for this change with the most up-to-date version of master branch? Thank you.

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Sep 15, 2020
@AndrewKushnir AndrewKushnir added target: patch This PR is targeted for the next patch release 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 Sep 15, 2020
….put()` overload

When the response type is JSON, the `put()` overload signature did not have `reportProgress`
and  `params` options. This makes it difficult to type-check this overload.

This commit adds them to the overload signature.

Fixes angular#23600
@ajitsinghkaler
Copy link
Contributor Author

@AndrewKushnir rebased it.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Sep 16, 2020
@AndrewKushnir
Copy link
Contributor

Hi @ajitsinghkaler, the presubmit went well. It looks like this PR has all approvals, so I'm adding the "merge" label. Thank you.

@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 Oct 17, 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: common/http cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpClient#put with observe: events and generic as return type doesn't take in reportProgress: true
7 participants