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

Allow specifying rootUrl at runtime #756

Merged
merged 1 commit into from Jun 16, 2017
Merged

Conversation

LegNeato
Copy link
Contributor

@LegNeato LegNeato commented Jun 6, 2017

This can be useful for testing (pointing clients at localhost) or proxying (if for some reason the proxy option is insufficient).

Note that I am still using buildurl, though I am only building url fragments. Perhaps it should be renamed to escapeurl, as it doesn't even really build a url...

I did not squash so you could see the template changes vs the generated api code changes. Let me know if you want me to do so.

  • npm test succeeds
  • Pull request has been squashed into 1 commit
  • I did NOT manually make changes to anything in apis/
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate JSDoc comments were updated in source code (if applicable)
  • Approprate changes to readme/docs are included in PR

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 6, 2017
@codecov-io
Copy link

Codecov Report

Merging #756 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #756   +/-   ##
=======================================
  Coverage   79.49%   79.49%           
=======================================
  Files           5        5           
  Lines         239      239           
  Branches       53       53           
=======================================
  Hits          190      190           
  Misses         30       30           
  Partials       19       19

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37347b5...fad8cab. Read the comment docs.

Copy link
Contributor

@jmdobry jmdobry left a comment

Choose a reason for hiding this comment

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

LGTM, go ahead and remove everything from your PR except your changes to templates/method-partial.ts and the test file.

@LegNeato LegNeato force-pushed the patch-1 branch 2 times, most recently from dfb03a4 to 5fcbc1e Compare June 16, 2017 02:34
@googlebot
Copy link
Collaborator

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jun 16, 2017
@LegNeato
Copy link
Contributor Author

LegNeato commented Jun 16, 2017

Ok, rebased on latest master and only did the commit with the test and template changes. 👍

Note that the tests will fail until APIs are regenerated...

This can be useful for testing (pointing clients at `localhost`) or proxying (if for some reason the `proxy` option is insufficient).

Test Plan:
* Added new test.
* `npm run generate-apis && npm run build && npm run test`
@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jun 16, 2017
@jmdobry
Copy link
Contributor

jmdobry commented Jun 16, 2017

Looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants