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

feat(geo): add support for geography #397

Merged
merged 14 commits into from
Apr 2, 2019
Merged

feat(geo): add support for geography #397

merged 14 commits into from
Apr 2, 2019

Conversation

steffnay
Copy link
Contributor

@steffnay steffnay commented Mar 24, 2019

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)

@steffnay steffnay requested a review from jkwlui March 24, 2019 16:21
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 24, 2019
@stephenplusplus
Copy link
Contributor

stephenplusplus commented Mar 24, 2019

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
Copy link
Contributor

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

@steffnay 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
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

LGTM, assuming callmehiphop signs off

src/index.ts Show resolved Hide resolved
system-test/.eslintrc.yml Outdated Show resolved Hide resolved
@steffnay steffnay changed the title fix(types): first attempt to add geo feat(geo): add support for geography Mar 25, 2019
@steffnay steffnay requested a review from callmehiphop March 25, 2019 16:35
Copy link
Contributor

@callmehiphop callmehiphop left a comment

Choose a reason for hiding this comment

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

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

@codecov
Copy link

codecov bot commented Mar 25, 2019

Codecov Report

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

Impacted file tree graph

@@            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
Copy link

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 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 steffnay requested a review from callmehiphop March 26, 2019 21:27
@jkwlui 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
@googlebot
Copy link

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.

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

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 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
@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Mar 30, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 31, 2019
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

LGTM assuming no new skipped tests are added

@JustinBeckwith 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
@googlebot
Copy link

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.

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

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 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
Copy link
Contributor

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

@callmehiphop callmehiphop added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 1, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 1, 2019
@sduskis 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
@googlebot
Copy link

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.

@callmehiphop 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 yoshi-automation added the 🚨 This issue needs some love. label Apr 2, 2019
@steffnay steffnay merged commit 8131b92 into googleapis:master Apr 2, 2019
@steffnay steffnay deleted the add-geo branch April 2, 2019 16:53
@JustinBeckwith
Copy link
Contributor

🎉

@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the pull request once unit tests and other checks pass. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants