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

fix(ts): provide complete and correct types #385

Merged
merged 7 commits into from
Apr 2, 2019
Merged

fix(ts): provide complete and correct types #385

merged 7 commits into from
Apr 2, 2019

Conversation

callmehiphop
Copy link
Contributor

@callmehiphop callmehiphop commented Mar 15, 2019

Fixes #332

This basically rewrites all the types. Pretty much every request/response interface was missing fields and every callback/response returned request.Response which we don't return anywhere.
In an effort to not make breaking changes, I've kept all type names and duplicate types in tact.

  • 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 Mar 15, 2019
@JustinBeckwith
Copy link
Contributor

Uh, is this a breaking change for TypeScript users?

@callmehiphop
Copy link
Contributor Author

@JustinBeckwith maybe? I'm not really sure how to classify changing interfaces that were flat out wrong.

@JustinBeckwith
Copy link
Contributor

Hah, I hear you. For now let's call it semver patch I guess?

@callmehiphop
Copy link
Contributor Author

@JustinBeckwith sounds good.. like I said in the overview my only concern would be about some fields being marked optional now. But I could probably just use a generic to mark them as non-optional until we're ready for a semver major.

@codecov
Copy link

codecov bot commented Mar 15, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
+ Coverage   99.44%   99.45%   +<.01%     
==========================================
  Files           4        4              
  Lines         544      547       +3     
  Branches       75       75              
==========================================
+ Hits          541      544       +3     
  Misses          2        2              
  Partials        1        1
Impacted Files Coverage Δ
src/index.ts 98.53% <100%> (ø) ⬆️
src/dataset.ts 100% <100%> (ø) ⬆️
src/table.ts 100% <100%> (ø) ⬆️
src/job.ts 100% <100%> (ø) ⬆️

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 2383b70...4d034a4. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 15, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
+ Coverage   99.45%   99.45%   +<.01%     
==========================================
  Files           4        4              
  Lines         547      550       +3     
  Branches       76       76              
==========================================
+ Hits          544      547       +3     
  Misses          2        2              
  Partials        1        1
Impacted Files Coverage Δ
src/index.ts 98.55% <100%> (ø) ⬆️
src/dataset.ts 100% <100%> (ø) ⬆️
src/table.ts 100% <100%> (ø) ⬆️
src/job.ts 100% <100%> (ø) ⬆️

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 c2ff0df...225e2f9. Read the comment docs.

@callmehiphop callmehiphop added do not merge Indicates a pull request not ready for merge, due to either quality or timing. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Mar 15, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 18, 2019
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

If I'm understanding @JustinBeckwith's comments correctly, and we're copying over TypeScript definitions from another-repo, I think it's worthwhile adding a build script that copies these over from the other library -- we'll be thankful in the future when the two libraries don't drift apart.

As for whether this is a breaking change; I'd argue, even if the old behavior is a bug, it's probably worth calling it a breaking change; especially since this library is already > 1.0.0; is it a huge deal if folks start getting [email protected]?

package.json Show resolved Hide resolved
src/dataset.ts Outdated Show resolved Hide resolved
src/table.ts Show resolved Hide resolved
src/types.d.ts Show resolved Hide resolved
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

👍 sounds like it would be a hassle to copy the types over; please do what you think is best @callmehiphop.

@JustinBeckwith JustinBeckwith self-requested a review March 21, 2019 22:24
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 22, 2019
@callmehiphop callmehiphop added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 26, 2019
@callmehiphop
Copy link
Contributor Author

There were a couple of names that were a little off, since we're going to release this as a major bump I went ahead and changed them.

@callmehiphop callmehiphop removed 🚨 This issue needs some love. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Mar 26, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 26, 2019
@JustinBeckwith JustinBeckwith added the automerge Merge the pull request once unit tests and other checks pass. label Mar 29, 2019
@callmehiphop callmehiphop merged commit 355d4d9 into googleapis:master Apr 2, 2019
@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.

Types (e.g. TableMetadata) disallow valid API usage
6 participants