X Tutup
Skip to content

fix(http): throw if url is not string or Request#4257

Closed
bkyarger wants to merge 1 commit intoangular:masterfrom
bkyarger:throw-exception-for-bad-request-input
Closed

fix(http): throw if url is not string or Request#4257
bkyarger wants to merge 1 commit intoangular:masterfrom
bkyarger:throw-exception-for-bad-request-input

Conversation

@bkyarger
Copy link
Copy Markdown
Contributor

@bkyarger bkyarger commented Sep 18, 2015

Closes #4245

@googlebot
Copy link
Copy Markdown

googlebot commented Sep 18, 2015

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@bkyarger bkyarger force-pushed the throw-exception-for-bad-request-input branch from f067824 to 1bbb41d Compare September 18, 2015 20:33
@googlebot
Copy link
Copy Markdown

googlebot commented Sep 18, 2015

CLAs look good, thanks!

@jeffbcross
Copy link
Copy Markdown
Contributor

jeffbcross commented Sep 18, 2015

@bkyarger FYI we're having issues with our CI environment right now, so some failures are expected. I'll restart jobs after the issues are resolved.

@bkyarger
Copy link
Copy Markdown
Contributor Author

bkyarger commented Sep 18, 2015

@jeffbcross sounds great. I'll check back tonight to see if there are still CI issues.

jeffbcross
jeffbcross reviewed Sep 18, 2015
View reviewed changes
modules/angular2/test/http/http_spec.ts Outdated
Copy link
Copy Markdown
Contributor

@jeffbcross jeffbcross Sep 18, 2015

Choose a reason for hiding this comment

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

Could you add the expected message here? toThrowError('First argument must be a string or Request.')

Copy link
Copy Markdown
Contributor Author

@bkyarger bkyarger Sep 18, 2015

Choose a reason for hiding this comment

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

Done

@jeffbcross jeffbcross self-assigned this Sep 18, 2015
@jeffbcross jeffbcross added type: bug/fix action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews comp: data-access labels Sep 18, 2015
@bkyarger bkyarger force-pushed the throw-exception-for-bad-request-input branch 2 times, most recently from a088529 to beca367 Compare September 18, 2015 21:40
jeffbcross
jeffbcross reviewed Sep 18, 2015
View reviewed changes
modules/angular2/test/http/http_spec.ts
Copy link
Copy Markdown
Contributor

@jeffbcross jeffbcross Sep 18, 2015

Choose a reason for hiding this comment

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

I'm surprised we don't have a Jsonp unit test for this method! Could you just duplicate this expectation in the same spec and call jsonp.request?

Copy link
Copy Markdown
Contributor Author

@bkyarger bkyarger Sep 18, 2015

Choose a reason for hiding this comment

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

Yep, will do.

Copy link
Copy Markdown
Contributor Author

@bkyarger bkyarger Sep 18, 2015

Choose a reason for hiding this comment

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

Added a new describe block for Jsonp, along with this same expectation for jsonp.

@bkyarger bkyarger force-pushed the throw-exception-for-bad-request-input branch from beca367 to 52c7dd8 Compare September 18, 2015 22:44
@jeffbcross
Copy link
Copy Markdown
Contributor

jeffbcross commented Sep 18, 2015

Awesome! LGTM. I'll try to merge it...our CI woes only affect PRs at the moment, so it should merge without issue.

@jeffbcross jeffbcross added pr_state: LGTM 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 labels Sep 18, 2015
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Sep 18, 2015

Merging PR #4257 on behalf of @jeffbcross to branch presubmit-jeffbcross-pr-4257.

@mary-poppins mary-poppins removed the action: merge The PR is ready for merge by the caretaker label Sep 18, 2015
@mhevery mhevery closed this in 3525d8a Sep 19, 2015
@bkyarger bkyarger deleted the throw-exception-for-bad-request-input branch September 19, 2015 00:20
robwormald pushed a commit to robwormald/angular that referenced this pull request Sep 25, 2015
@robwormald
fix(http): throw if url is not string or Request
e9def91 
Closes angular#4245

Closes angular#4257
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Sep 7, 2019

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 Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@jeffbcross jeffbcross

Labels

cla: yes type: bug/fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix(http): should throw an exception if input to http.request is not string or Request

4 participants

@bkyarger @googlebot @jeffbcross @mary-poppins
X Tutup