-
Notifications
You must be signed in to change notification settings - Fork 85
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: correct AppendSerializtionError typo #2037
Conversation
cc: @suztomo |
"ci / clirr" passing is a nice. |
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 think there are 2 options: class AppendSerializationError extends AppendSerializtionError
(super class has typo) or class AppendSerializtionError extends AppendSerializationError
(base class has typo).
This pull request seems to have chosen the former. Would you write down the rationale?
@@ -244,6 +244,17 @@ public String getStreamName() { | |||
} | |||
} | |||
|
|||
public static class AppendSerializationError extends AppendSerializtionError { |
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.
Javadoc? (I expect a copy of AppendSerializtionError) Explanation of typo and backward compatibility is nice.
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.
Since we don't want to break customers using the AppendSerializationError(with typo) and at the same time, move all the reference we have to that class in the library, to the class without typo, it made sense to use class AppendSerializationError extends AppendSerializtionError
(super class has typo).
The other way round would mean, we move the code in AppendSerializtionError
to AppendSerializationError
for us to use in the library. That would break the external customers, if we don't move the code, that will cause code duplication.
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.
That makes sense.
@suztomo Would you mind taking another look? |
Let's say you merge this. How would library users know they use |
Customers who are not directly using this class will not be affected in any way. Any customer who is directly using the AppendSerializtionError(with typo) will continue to use it but should eventually move to AppendSerializationError(without typo), if we remove the class with typo. The purpose of doing this work around right now is to introduce the class without typo, without introducing a breaking change now because we don't want a major version update now. Eventually when we do a major version update, we should remove the class with typo. This will break customers who are directly using the class with typo. There is no way around it. For any users who may want to use the class from now, there is a warning on the class with typo that it will be removed. I could add a @deprecated annotation as well. |
@yirutang PTAL |
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 also think this is the best under the restriction of no breaking changes.
google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/Exceptions.java
Show resolved
Hide resolved
* fix: correct AppendSerializtionError typo * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * chore: add javadoc * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [2.34.2](https://togithub.com/googleapis/java-bigquerystorage/compare/v2.34.1...v2.34.2) (2023-03-30) ### Bug Fixes * Correct AppendSerializtionError typo ([#2037](https://togithub.com/googleapis/java-bigquerystorage/issues/2037)) ([e67e913](https://togithub.com/googleapis/java-bigquerystorage/commit/e67e913f34fda4f4cc523c0248e5344232c0b736)) ### Dependencies * Update dependency com.google.cloud:google-cloud-bigquery to v2.24.0 ([#2054](https://togithub.com/googleapis/java-bigquerystorage/issues/2054)) ([e3156c7](https://togithub.com/googleapis/java-bigquerystorage/commit/e3156c7b525f7df2f3fe756f096e7fb1352fae8e)) * Update dependency com.google.cloud:google-cloud-bigquery to v2.24.1 ([#2056](https://togithub.com/googleapis/java-bigquerystorage/issues/2056)) ([a989ac6](https://togithub.com/googleapis/java-bigquerystorage/commit/a989ac63d813cc98dcc13200a950fe3edad10bdf)) * Update dependency com.google.cloud:google-cloud-bigquery to v2.24.3 ([#2058](https://togithub.com/googleapis/java-bigquerystorage/issues/2058)) ([9346667](https://togithub.com/googleapis/java-bigquerystorage/commit/934666737a92ec3220c6a186cc1af0f1adabb00c)) * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.6.0 ([#2063](https://togithub.com/googleapis/java-bigquerystorage/issues/2063)) ([965de7b](https://togithub.com/googleapis/java-bigquerystorage/commit/965de7bf78884cca30e6e6d672b74d734bda840d)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Since we don't want to break customers using the
AppendSerializtionError
(with typo) and at the same time, move all the reference we have to that class in the library, to the class without typo, it made sense to use classAppendSerializationError extends AppendSerializtionError
(super class has typo).The other way round would mean, we move the code in
AppendSerializtionError
toAppendSerializationError
for us to use in the library. That would break the external customers, if we don't move the code, that will cause code duplication.