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.

Sign up for GitHub

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

Jump to bottom

gapic: doc.go refers to "NewClient" instead of service-specific constructors. #1077

Closed
timburks opened this issue Jul 19, 2022 · 3 comments · Fixed by #1099 or #1089
Closed

gapic: doc.go refers to "NewClient" instead of service-specific constructors. #1077

timburks opened this issue Jul 19, 2022 · 3 comments · Fixed by #1099 or #1089
Assignees
@quartzmo
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@timburks
Copy link
Contributor

Reviewing this code, it appears that client constructors include the names of their associated services.

But here, a comment in generated doc.go refers to an initializer named NewClient. Is that an obsolete reference?
@timburks timburks added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Jul 19, 2022
@timburks timburks mentioned this issue Jul 19, 2022
Merged
@noahdietz
Copy link
Collaborator

noahdietz commented Jul 19, 2022 •
edited
Loading

Not an obsolete reference, but a naive one. It should be updated to include the %s like the first link mentions.

The name that (potentially) goes in there is a reduction of the proto service name i.e. if the package is language and the proto service is named Language the factory method for that client will be NewClient (%s resolves to ""), but if the package is named language and the proto service is named FooBar, the factory method would be NewFooBarClient. This prevents stuttering like language.NewLanguageClient and language.LanguageClient.

You'll notice in your first link that the value given to the %s is something like servName - that is the reduced service name. Would happily take a fix for this, but also perhaps @quartzmo would be interested in fixing it. Either way, let us know @timburks, thanks for catching that.

@timburks
Copy link
Contributor Author

Thanks @noahdietz - I'd be interested in fixing this but can't get to it for at least the next few weeks so let's leave it up for grabs. I'll follow up if/when I start any work on it.

@noahdietz
Copy link
Collaborator

@quartzmo any interest in making this generated doc fix?

@quartzmo quartzmo self-assigned this Aug 4, 2022
quartzmo added a commit to quartzmo/gapic-generator-go that referenced this issue Aug 4, 2022
@quartzmo
fix(gengapic): fix service-specific constructor name in doc_file.go
852bc34 
closes: googleapis#1077
@quartzmo quartzmo mentioned this issue Aug 4, 2022
Merged
gcf-merge-on-green bot pushed a commit that referenced this issue Aug 4, 2022
@quartzmo
fix(gengapic): fix service-specific constructor name in doc_file.go (#…
4f80726 
…1099)

closes: #1077
@release-please release-please bot mentioned this issue Aug 4, 2022
Merged
gcf-merge-on-green bot pushed a commit that referenced this issue Aug 16, 2022
@release-please
chore(main): release 0.32.0 (#1089)
a4d13ee 
🤖 I have created a release *beep* *boop*
---


## [0.32.0](v0.31.2...v0.32.0) (2022-08-16)


### Features

* **gengapic:** rest-numeric-enums option enables enum-encoding sys param ([#1022](#1022)) ([6bbbf6f](6bbbf6f))


### Bug Fixes

* **gengapic:** fix linkParser regexp to support multi-line link tags ([#1098](#1098)) ([863675e](863675e)), closes [#1097](#1097)
* **gengapic:** fix service-specific constructor name in doc_file.go ([#1099](#1099)) ([4f80726](4f80726)), closes [#1077](#1077)

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@quartzmo quartzmo

Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
3 participants
@timburks @quartzmo @noahdietz