-
Notifications
You must be signed in to change notification settings - Fork 373
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(speech): generate speech v2 #10228
Conversation
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov ReportBase: 93.84% // Head: 93.84% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #10228 +/- ##
==========================================
- Coverage 93.84% 93.84% -0.01%
==========================================
Files 1551 1551
Lines 142784 142784
==========================================
- Hits 134000 133998 -2
- Misses 8784 8786 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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 see no changes the main.dox
which would include the samples for this new client. I think you will need to make some changes to the ci/generate-markdown
scripts and maybe add new placeholders in main.dox
.
Likewise, you need a new group for the Options documentation.
Finally, the samples are not getting compiled. You need to add a BUILD and CMakeLists.txt file (the scaffold steps would have done that before). And you need to modify the top-level CMake file around here:
google-cloud-cpp/CMakeLists.txt
Lines 358 to 365 in 1fbb8eb
if (GOOGLE_CLOUD_CPP_ENABLE_EXAMPLES | |
AND IS_DIRECTORY | |
"${CMAKE_CURRENT_SOURCE_DIR}/google/cloud/${feature}/samples" | |
AND EXISTS | |
"${CMAKE_CURRENT_SOURCE_DIR}/google/cloud/${feature}/samples/CMakeLists.txt" | |
) | |
add_subdirectory(google/cloud/${feature}/samples) | |
endif () |
google/cloud/speech/CMakeLists.txt
Outdated
@@ -50,10 +50,24 @@ google_cloud_cpp_grpcpp_library( | |||
external_googleapis_set_version_and_alias(speech_protos) | |||
target_link_libraries(google_cloud_cpp_speech_protos PUBLIC ${proto_deps}) | |||
|
|||
unset(mocks_globs) | |||
unset(source_globs) | |||
set(service_dirs ";v2/") |
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.
if you are planning to use something like a list, then do so:
set(service_dirs "" "v2/")
set(internal_dirs "" "internal/")
set(extensions "*.h" ".cc")
But all of this seems overly complicated, I think something like
file(GLOB source_files RELATIVE "${CMAKE_CURRENT_SOURCE_DIR}"
"*.h" "*.cc" "internal/*.h" "intermal/*.cc"
"v2/*.h" "v2/*.cc" "v2/internal/*.h" "v2/intermal/*.cc")
Is easier to read.
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.
Ack, I unrolled the internal_dirs
and extensions
, but I left the service_dirs
loop. I think it offers a general solution that we can use in other libraries. e.g. bigquery might (eventually) look like:
set(service_dirs "analyticshub/v1/" "connection/v1/"
"datatransfer/v1/" "migration/v2/"
"reservation/v1/" "storage/v1/" "v2/")
/** | ||
* Use with `google::cloud::Options` to configure the retry policy. | ||
* | ||
* @ingroup google-cloud-speech-v2-options |
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 new Doxygen group, you need to create a page to host these.
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.
Discussed offline. Fixed by #10238 + regeneration
4535e8b
to
a890aac
Compare
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Fixed by #10241 + regeneration |
@@ -42,6 +42,10 @@ which should give you a taste of the Cloud Speech-to-Text API C++ client library | |||
|
|||
<!-- inject-endpoint-env-vars-start --> | |||
|
|||
- `GOOGLE_CLOUD_CPP_SPEECH_ENDPOINT=...` overrides the |
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.
Seems like some sort of sort -u
would be useful in the script that updates these things?
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 lean more towards including the namespace, such as: speech_v2::MakeSpeechConnection()
Fixes #10168
Note that moving speech v1 is not a prerequisite for making this change. The types will not conflict, and we know where we want to put speech v2.
I tried to come up with a general glob solution. It deserves scrutiny.
This change is