feat(geo): add support for geography by steffnay · Pull Request #397 · googleapis/nodejs-bigquery

steffnay · 2019-03-24T16:21:23Z

Trying to locate which portions of code need to be touched. tests totally failing!
Just seeking feedback on how this should actually be done.

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

…d to be touched. tests totally failing

stephenplusplus · 2019-03-24T17:16:29Z

It looks like the lint task is being a bit faily. You could try running npm run fix to sort out the issues and then see if there are any issues left. 🤞

> @google-cloud/[email protected] posttest /tmpfs/src/github/nodejs-bigquery
> npm run check
> @google-cloud/[email protected] check /tmpfs/src/github/nodejs-bigquery
> gts check
/tmpfs/src/github/nodejs-bigquery/system-test/bigquery.ts
  Lines: 1309-1321
          table = dataset.table(generateName('table'));
          return table.create({
            schema: [
   -          'date:DATE',
   -          'datetime:DATETIME',
   -          'time:TIME',
   -          'timestamp:TIMESTAMP',
   -          'numeric:NUMERIC',
   -          'geography:GEOGRAPHY'
   +          'date:DATE', 'datetime:DATETIME', 'time:TIME', 'timestamp:TIMESTAMP',
   +          'numeric:NUMERIC', 'geography:GEOGRAPHY'
            ].join(', '),
          });
        });
clang-format reported errors... run `gts fix` to address.

- Build log

callmehiphop · 2019-03-25T07:24:01Z

We should add some tests to the SQL Parameter system-tests.

JustinBeckwith

LGTM, assuming callmehiphop signs off

callmehiphop

Looks good, I think all we're missing are some tests. We should make sure that we can

use Geography as a SQL Parameter
unit test that GEOGRAPHY returns a Geography object.

codecov · 2019-03-25T23:09:23Z

Codecov Report

Merging #397 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #397      +/-   ##
==========================================
+ Coverage   99.45%   99.46%   +<.01%     
==========================================
  Files           4        4              
  Lines         550      556       +6     
  Branches       76       76              
==========================================
+ Hits          547      553       +6     
  Misses          2        2              
  Partials        1        1

Impacted Files	Coverage Δ
src/table.ts	`100% <ø> (ø)`	⬆️
src/index.ts	`98.59% <100%> (+0.04%)`	⬆️

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 355d4d9...89aca53. Read the comment docs.

googlebot · 2019-03-25T23:09:24Z

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 or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok 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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-28T21:19:10Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-28T21:33:11Z

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 or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok 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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

…into add-geo

JustinBeckwith

LGTM assuming no new skipped tests are added

googlebot · 2019-04-01T16:55:21Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-04-01T20:36:31Z

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 or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok 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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

callmehiphop · 2019-04-01T21:46:48Z

@JustinBeckwith @jkwlui can one of you please defeat the cursed cla 🤖 megaboss?

googlebot · 2019-04-02T16:35:51Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

JustinBeckwith · 2019-04-02T16:54:41Z

🎉

first attempt to add geo. trying to locate which portions of code nee…

317975b

…d to be touched. tests totally failing

steffnay requested a review from jkwlui March 24, 2019 16:21

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 24, 2019

linted

4673901

steffnay changed the title ~~first attempt to add geo. trying to locate which portions of code need to be touched. tests totally failing!~~ fix(types): first attempt to add geo Mar 25, 2019

JustinBeckwith approved these changes Mar 25, 2019

View reviewed changes

Comment thread src/index.ts

Comment thread system-test/.eslintrc.yml Outdated

steffnay changed the title ~~fix(types): first attempt to add geo~~ feat(geo): add support for geography Mar 25, 2019

steffnay requested a review from callmehiphop March 25, 2019 16:35

callmehiphop suggested changes Mar 25, 2019

View reviewed changes

callmehiphop added 2 commits March 25, 2019 19:07

exclude geography from being promisified

9f0be52

add geography to custom class list for encoding values

7658d02

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 Mar 25, 2019

steffnay added 2 commits March 25, 2019 20:05

add unit and system tests for geography

39e003b

Added test

24aacef

steffnay requested a review from callmehiphop March 26, 2019 21:27

jkwlui 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 Mar 28, 2019

steffnay marked this pull request as ready for review March 28, 2019 21:30

Merge branch 'master' into add-geo

5a1a994

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 Mar 28, 2019

steffnay added 2 commits March 28, 2019 14:44

Merge branch 'master' of https://github.com/googleapis/nodejs-bigquery …

8b61f3c

…into add-geo

Merge branch 'add-geo' of https://github.com/steffnay/nodejs-bigquery …

bae619c

…into add-geo

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Mar 30, 2019

yoshi-automation added the 🚨 This issue needs some love. label Mar 31, 2019

JustinBeckwith approved these changes Apr 1, 2019

View reviewed changes

JustinBeckwith added cla: yes This human has signed the Contributor License Agreement. and removed 🚨 This issue needs some love. cla: no This human has *not* signed the Contributor License Agreement. labels Apr 1, 2019

yoshi-automation added the 🚨 This issue needs some love. label Apr 1, 2019

update geography tests, removed skipped test

62fc5b0

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 Apr 1, 2019

callmehiphop added 2 commits April 1, 2019 17:40

add unit test for geography instance method

c101f85

add trailing commas

0775162

callmehiphop approved these changes Apr 1, 2019

View reviewed changes

callmehiphop added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 1, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 1, 2019

Merge branch 'master' into add-geo

89aca53

sduskis 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 Apr 2, 2019

callmehiphop added automerge Merge the pull request once unit tests and other checks pass. and removed 🚨 This issue needs some love. labels Apr 2, 2019

yoshi-automation added the 🚨 This issue needs some love. label Apr 2, 2019

steffnay merged commit 8131b92 into googleapis:master Apr 2, 2019

steffnay deleted the add-geo branch April 2, 2019 16:53

yoshi-automation removed the 🚨 This issue needs some love. label Apr 6, 2020

release-please Bot mentioned this pull request May 18, 2022

chore(main): release 6.0.0 #1117

Merged

Conversation

steffnay commented Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenplusplus commented Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

callmehiphop commented Mar 25, 2019

Uh oh!

JustinBeckwith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

callmehiphop left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

googlebot commented Mar 25, 2019

Uh oh!

googlebot commented Mar 28, 2019

Uh oh!

googlebot commented Mar 28, 2019

Uh oh!

JustinBeckwith left a comment

Choose a reason for hiding this comment

Uh oh!

googlebot commented Apr 1, 2019

Uh oh!

googlebot commented Apr 1, 2019

Uh oh!

callmehiphop commented Apr 1, 2019

Uh oh!

googlebot commented Apr 2, 2019

Uh oh!

JustinBeckwith commented Apr 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

steffnay commented Mar 24, 2019 •

edited

Loading

stephenplusplus commented Mar 24, 2019 •

edited

Loading

codecov Bot commented Mar 25, 2019 •

edited

Loading