Skip to content

feat: add support for appengine legacy url encoding #456

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Aug 5, 2019
Merged

feat: add support for appengine legacy url encoding #456

merged 10 commits into from
Aug 5, 2019

Conversation

@jiren
Copy link
Member

@jiren jiren commented Jul 9, 2019

Fixes #152 and #105

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 9, 2019
@JustinBeckwith
Copy link
Contributor

JustinBeckwith commented Jul 9, 2019

Thanks for the PR! @crwilcox & @stephenplusplus could I trouble y'all to take a look here?

@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2019
@codecov
Copy link

codecov bot commented Jul 10, 2019 •
edited
Loading

Codecov Report

Merging #456 into master will increase coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
+ Coverage   98.12%   98.24%   +0.12%     
==========================================
  Files           5        5              
  Lines         639      684      +45     
  Branches      153      159       +6     
==========================================
+ Hits          627      672      +45     
  Partials       12       12
Impacted Files Coverage Δ
src/entity.ts 98.56% <100%> (+0.25%) ⬆️
src/index.ts 98.71% <100%> (+0.05%) ⬆️

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 771b634...96754f3. Read the comment docs.

stephenplusplus
stephenplusplus reviewed Jul 10, 2019
View reviewed changes
src/utils.ts Outdated
@@ -0,0 +1,164 @@
import * as Protobuf from 'protobufjs';
Copy link
Contributor

@stephenplusplus stephenplusplus Jul 10, 2019

Choose a reason for hiding this comment

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

Could these new classes be moved to src/entity.ts?

Copy link
Member Author

@jiren jiren Jul 11, 2019

Choose a reason for hiding this comment

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

ok

Copy link
Member Author

@jiren jiren Jul 11, 2019

Choose a reason for hiding this comment

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

@stephenplusplus Moved into entity.ts

@jiren
Moved urlsafekey class to entity
e6a8ea8 
- updated test cases
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 15, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 15, 2019
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 15, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 15, 2019
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 23, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 23, 2019
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 26, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 26, 2019
@stephenplusplus
Copy link
Contributor

stephenplusplus commented Aug 1, 2019

@jiren looks like there is an issue with the samples test: https://source.cloud.google.com/results/invocations/f307d150-cef8-4d65-bb63-714d9931c311/targets/cloud-devrel%2Fclient-libraries%2Fnodejs%2Fpresubmit%2Fgoogleapis%2Fnodejs-datastore%2Fnode10%2Fsamples-test/log -- I'm not sure how this could be related to your changes, but other PRs don't seem to have the same failure. Any ideas?

@jiren
Copy link
Member Author

jiren commented Aug 2, 2019

@stephenplusplus looking into it.

@JustinBeckwith JustinBeckwith changed the title Added support for appengine legacy url encoding feat: add support for appengine legacy url encoding Aug 5, 2019
@JustinBeckwith JustinBeckwith merged commit 357a9cf into googleapis:master Aug 5, 2019
@jiren jiren deleted the safe_url_105 branch August 6, 2019 06:14
@AVaksman AVaksman mentioned this pull request Aug 13, 2019
Closed
yoshi-automation added a commit that referenced this pull request Apr 1, 2020
@yoshi-automation
fix: do not run node 8 CI (#456)
9cc58a6 
googleapis/synthtool@1b4cc80
commit 1b4cc80a7aaf164f6241937dd87f3bd1f4149e0c
Author: Alexander Fenster <fenster@google.com>
Date:   Wed Mar 25 08:01:31 2020 -0700

    fix: do not run node 8 CI (#456)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@stephenplusplus stephenplusplus stephenplusplus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Query datastore entity by url-safe key or decode it to a searchable key

6 participants

@jiren @JustinBeckwith @stephenplusplus @googlebot @AVaksman @kokoro-team