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!: migrate to use microgen #23

Merged
merged 3 commits into from Aug 28, 2020
Merged

feat!: migrate to use microgen #23

merged 3 commits into from Aug 28, 2020

Conversation

@arithmetic1728
Copy link
Contributor

@arithmetic1728 arithmetic1728 commented Aug 20, 2020

(1) Migrate the auto-generated errorreporting_v1beta1 to use microgenerator.
(2) There is no interface changes to the handwritten layer, so users won't be affected.
(3) bump the major version because this PR drops python 2.7 support.

@daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Aug 26, 2020

I noticed there are now 3 modules: google.cloud.error_reporting, google.cloud.errorreporting, and google.cloud.errorreporting_v1beta1. errorreporting is mostly empty. Is that intentional?

Loading

@arithmetic1728
Copy link
Contributor Author

@arithmetic1728 arithmetic1728 commented Aug 26, 2020

I noticed there are now 3 modules: google.cloud.error_reporting, google.cloud.errorreporting, and google.cloud.errorreporting_v1beta1. errorreporting is mostly empty. Is that intentional?

Yes, errorreporting module is a wrapper of errorreporting_v1beta1, they are both auto-generated and users can use either module.

Loading

@daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Aug 26, 2020

Yes, errorreporting module is a wrapper of errorreporting_v1beta1, they are both auto-generated and users can use either module.

Isn't that going to be a bad developer experience for the user? That we expose both errorreporting and error_reporting with one character different but separate APIs? And then giving them errorreporting_v1beta1 as a third option? I'm new to this so please explain if I'm thinking about it wrong, but that seems very confusing to me.

Loading

@arithmetic1728
Copy link
Contributor Author

@arithmetic1728 arithmetic1728 commented Aug 26, 2020

Yes, errorreporting module is a wrapper of errorreporting_v1beta1, they are both auto-generated and users can use either module.

Isn't that going to be a bad developer experience for the user? That we expose both errorreporting and error_reporting with one character different but separate APIs? And then giving them errorreporting_v1beta1 as a third option? I'm new to this so please explain if I'm thinking about it wrong, but that seems very confusing to me.

For all client libraries, we have one folder for each version, for instance, foo_v1alpha1, foo_v1beta1, then we have a wrapper foo to wrap the default version. It is unfortunate that in this library, the handwritten folder error_reporting is very similar to the wrapper name.

Loading

@daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Aug 26, 2020

wrapper foo to wrap the default version. It is unfortunate that in this library, the handwritten folder error_reporting is very similar to the wrapper name.

I see, so errorreporting is just the alias for error_reporting_v1beta1, and error_reporting is the handwritten layer? Is that something we can change? Maybe change error_reporting -> errorreporting_vaneer and integrate all endpoints into the root erroreporting? (I understand this is getting out of scope of your PR, but curious if you know how other libraries handle it)

Loading

@arithmetic1728
Copy link
Contributor Author

@arithmetic1728 arithmetic1728 commented Aug 26, 2020

wrapper foo to wrap the default version. It is unfortunate that in this library, the handwritten folder error_reporting is very similar to the wrapper name.

I see, so errorreporting is just the alias for error_reporting_v1beta1, and error_reporting is the handwritten layer?

yes

Is that something we can change? Maybe change error_reporting -> errorreporting_vaneer and integrate all endpoints into the root erroreporting? (I understand this is getting out of scope of your PR, but curious if you know how other libraries handle it)

As far as I understand, users don't use the auto generated layer at all so this shouldn't be a problem.

Loading

@busunkim96
Copy link
Collaborator

@busunkim96 busunkim96 commented Aug 26, 2020

Echoing what @simonz130 said earlier in a chat thread, error reporting is an API that is probably too convoluted to use without the wrapper. See this bit of code for example that unrolls a traceback into the format the APi expects.

We could remove the unversioned alias if you think it's likely to cause confusion.

Loading

@arithmetic1728
Copy link
Contributor Author

@arithmetic1728 arithmetic1728 commented Aug 27, 2020

Echoing what @simonz130 said earlier in a chat thread, error reporting is an API that is probably too convoluted to use without the wrapper. See this bit of code for example that unrolls a traceback into the format the APi expects.

We could remove the unversioned alias if you think it's likely to cause confusion.

I can remove the alias if you guys would like so.

Loading

@daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Aug 27, 2020

I can remove the alias if you guys would like so.

That would be my vote, but I'll defer to @simonz130 if he has other opinions

I still wonder if there would be a better way to handle hand-written vs generated packages though. Even if it's not an issue for this one, it may come up with other libraries. (It took me a while to realize that logging and logging_v2 weren't two versions of the logging library, but two separate libraries with different use cases) But I can save that discussion for later!

Loading

@arithmetic1728
Copy link
Contributor Author

@arithmetic1728 arithmetic1728 commented Aug 27, 2020

I just updated the PR to remove the google/cloud/errorreporting folder so the folder structures remain the same as before. Please let me know if you have any other comments regarding this PR. Thanks!

Loading

Copy link
Contributor

@daniel-sanche daniel-sanche left a comment

LGTM

Loading

@arithmetic1728 arithmetic1728 merged commit cb41e3a into master Aug 28, 2020
8 checks passed
Loading
@arithmetic1728 arithmetic1728 deleted the sijun branch Aug 28, 2020
@release-please release-please bot mentioned this pull request Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants