-
Notifications
You must be signed in to change notification settings - Fork 210
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
Release @google-cloud/bigquery v2.1.1 #396
Conversation
Codecov Report
@@ Coverage Diff @@
## master #396 +/- ##
=======================================
Coverage 99.45% 99.45%
=======================================
Files 4 4
Lines 547 547
Branches 76 76
=======================================
Hits 544 544
Misses 2 2
Partials 1 1 Continue to review full report at Codecov.
|
|
||
### Bug Fixes | ||
- fix: correctly encode nested custom date/time parameters ([#393](https://github.com/googleapis/nodejs-bigquery/pull/393)) | ||
- fix(job): check for `errorResult` when polling jobs ([#387](https://github.com/googleapis/nodejs-bigquery/pull/387)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tricky kind of fix, since the interpretation against semver isn't always a clear win for the user. This fix could be breaking change-ish for how the user has handled Jobs in the past.
Before: Job emits 'error'.
After: Job completes successfully.
We don't really know what the user does in each of those scenarios, although it seems relatively safe to fail less for the user.
So I don't know if we should bump the major for this (anyone think we should?), but at least calling it out in the docs release notes might be helpful to someone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You make a good point and I believe @bcoe shares a similar opinion. I would be curious to hear what others think as well. While this fix is breaking-ish, it's only because it wasn't implemented correctly to begin with. I think that implies users weren't expecting the code to behave the way it did and that a patch would be appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I would lean towards calling this a breaking change:
- if you're making an effort to adhere to semver, it's important to move away from attaching too much meaning to the major digit; changing the major just means you've made a breaking API change, it doesn't need to correlate with an exciting new release of your application.
- @google-cloud/bigquery has enough users that I'd bet at least one of them was relying on the fact that results would populate even though an error was occurring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcoe all sounds reasonable to me! I've never had much of a say in our breaking change policies, so I would delegate to @JustinBeckwith on that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I think this functionality has been broken since 2.0.4
(December) and all body errors have been ignored entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's compromise and call out the change in the CHANGELOG, as:
Note: a bug was fixed that some people may have been relying on, results were still be populated in error
responses when an API returned more than the maximum configured errors.
I think we need to better define our policies around breaking API changes, in relation to fixes
, and I don't want to block this release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to drive some one crazy, but I'm ok trying to make a major release (assuming no one objects) and also trying to squeeze #385 in with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM. We closing this out then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote for calling this 3.0.0
, but could be convinced that it's a safe enough change that it's unlikely to disrupt our users (and need not be a major).
This pull request was generated using releasetool.