-
Notifications
You must be signed in to change notification settings - Fork 46
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
chore: transition the library to microgenerator #62
Conversation
c252fba
to
38e6c39
Compare
Not all paths are covered, not even in the generated code, thus the adjustment is necessary.
# limitations under the License. | ||
# | ||
|
||
from .services.big_query_read import BigQueryReadClient |
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 have this be the manual wrapper.
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 generated file and bigquery_storage_v1/client.py
imports from it. The latter assumes that the generated client is imported and wraps it, although it could also import directly from storage_v1.services.big_query_read
, in principle.
Do you think it's worth maintaining additional synth replacement rules to prevent users from accessing the generated client through google.cloud.bigquery.storage_v1
?
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.
Do you think it's worth maintaining additional synth replacement rules to prevent users from accessing the generated client through google.cloud.bigquery.storage_v1?
I'm pretty sure that's what I did for the bigquery_storage_v1beta1
package (that is, exclude the equivalent top-level __init__.py
from getting copied over.
I'm still flip-flopping about what namespace we should use for the manual clients. It's nice to avoid breaking changes, but if we're breaking anyway, maybe we should be consistent. What do other manual libs do for namespace of manual clients?
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.
PubSub just exposes the generated clients through pubsub_v1.__init__.py
import.
I don't know about other libs, but can check (I suspect they left these imports intact, though).
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.
Interesting! I find that it uses the google.pubsub_v1
for generated and google.cloud.pubsub_v1
for handwritten to be pretty confusing, but I guess so long as it's documented well, it'll be okay.
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 can at least mention this in the UPGRADING guide and just recommend to import from google.cloud.bigquery.storage
(the samples use this path, too). This will give us more freedom to restructure the library internally in the future (in theory at least).
Update: FWIW, I found another library with hand-written layer that has already been migrated to microgenerator (AutoML) and it, too, exposes the generated client through google.cloud.automl_v1
.
Rules conditionally matching versions other than v1 are not needed anymore.
Addressed all comments. Once the changes are finalized and confirmed, I will follow up with the UPGRADING guide. Update: Added the upgrade guide, as I think nothing significant will change anymore now that we discussed the import paths. |
Regarding the Sample tests, I just sent out internal CL 333505519 to add the missing Kokoro config. |
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.
LGTM once those UPGRADING fixes are in.
Closes #61.
Remaining things:
v1beta1
andv1beta2
code (remove? YES!).