feat(developers/knowledge): onboard v1 library#5970
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds a new generated client library crate, google-developers-knowledge-v1, for the Developer Knowledge API, along with its corresponding configurations in Cargo.toml, Cargo.lock, and librarian.yaml. Feedback on the changes highlights a markdown formatting typo in the generated documentation within model.rs that should be addressed in the upstream generator or proto definition to prevent rendering issues.
| /// * `update_time < "2024-01-01T00:00:00Z"` | ||
| /// * `update_time >= "2025-01-22T00:00:00Z" AND (data_source = | ||
| /// "developer.chrome.com" OR data_source = "web.dev")` | ||
| /// * `uri = `https://docs.cloud.google.com/release-notes`` |
There was a problem hiding this comment.
There is a markdown formatting typo at the end of this line with a trailing double backtick ( `uri` = `https://docs.cloud.google.com/release-notes ``). This will cause rendering issues in the generated rustdoc. Since this is generated code, please fix this typo in the generator or the upstream proto definition.
There was a problem hiding this comment.
Blegh, you may want to run cargo doc -p google-developers-knowledge-v1 to make sure this compiles. Rust is notoriously strict about markdown, and we only run the documentation tests post-build.
There was a problem hiding this comment.
I get the following with exit code 0 (it seems to run on everything even though I specified the one crate...I did run it from root):
warning: unresolved link to `crate::grpc::Client`
--> src/gax-internal/src/observability/client_signals/recorder.rs:85:19
|
85 | /// [GrpcClient]: crate::grpc::Client
| ^^^^^^^^^^^^^^^^^^^ no item named `grpc` in module `google_cloud_gax_internal`
|
= note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default
warning: `google-cloud-gax-internal` (lib doc) generated 1 warning
Documenting google-developers-knowledge-v1 v1.0.0 (/usr/local/google/home/ndietz/rust/google-cloud-rust/src/generated/developers/knowledge/v1)
warning: this URL is not a hyperlink
--> src/generated/developers/knowledge/v1/src/model.rs:271:19
|
271 | /// * `uri = `https://docs.cloud.google.com/release-notes``
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: bare URLs are not automatically turned into clickable links
= note: `#[warn(rustdoc::bare_urls)]` on by default
help: use an automatic link instead
|
271 | /// * `uri = `<https://docs.cloud.google.com/release-notes>``
| + +
warning: `google-developers-knowledge-v1` (lib doc) generated 1 warning (run `cargo fix --lib -p google-developers-knowledge-v1` to apply 1 suggestion)
Will Warning findings cause problems?
There was a problem hiding this comment.
I get the following with exit code
0(it seems to run on everything even though I specified the one crate...I did run it from root):
warning: unresolved link to `crate::grpc::Client` --> src/gax-internal/src/observability/client_signals/recorder.rs:85:19 | 85 | /// [GrpcClient]: crate::grpc::Client | ^^^^^^^^^^^^^^^^^^^ no item named `grpc` in module `google_cloud_gax_internal` | = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default warning: `google-cloud-gax-internal` (lib doc) generated 1 warning
This is unrelated... filed #5971 to fix.
Documenting google-developers-knowledge-v1 v1.0.0 (/usr/local/google/home/ndietz/rust/google-cloud-rust/src/generated/developers/knowledge/v1) warning: this URL is not a hyperlink --> src/generated/developers/knowledge/v1/src/model.rs:271:19 | 271 | /// * `uri = `https://docs.cloud.google.com/release-notes`` | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: bare URLs are not automatically turned into clickable links = note: `#[warn(rustdoc::bare_urls)]` on by default help: use an automatic link instead | 271 | /// * `uri = `<https://docs.cloud.google.com/release-notes>`` | + + warning: `google-developers-knowledge-v1` (lib doc) generated 1 warning (run `cargo fix --lib -p google-developers-knowledge-v1` to apply 1 suggestion)Will
Warningfindings cause problems?
The CI will break, we compile with -D warnings (aka "deny warnings"):
google-cloud-rust/.gcb/scripts/docs.sh
Line 27 in 90859cf
We can configure the generator to disable warnings, for example:
google-cloud-rust/librarian.yaml
Lines 115 to 117 in 90859cf
If you allow me to ramble for a second:
- We may want to disable this bare url warnings for all libraries.
- Fixing the markdown upstream is hopeless.
- We could implement sidekick: use goldmark's linkify to automatically process links librarian#1600 to get better link handling, what we have is a bit of a hack
- I think we need to change something about or CI or playbook to compile the docs in the
librarian addPR, or not, and live with the post-merge build error and the super urgent bug that follows.
There was a problem hiding this comment.
I think we need to change something about our CI or playbook to compile the docs in the librarian add PR
Not mutually exclusive - CI should verify this, but humans shouldn't be surprised by it. So we should add it to CI and also work it into the playbook.
We could implement sidekick: use goldmark's linkify to automatically process links librarian#1600 to get better link handling, what we have is a bit of a hack
Do you think that the juice will be worth the squeeze vs. just disabling this warning across libraries ? We could of course do both, raising the floor with better link handling, but also not blocking things on unlinkable, garbage proto comments. I'm not sure how hard it would be to implement this fix nor how much more effective it would be.
We may want to disable this bare url warnings for all libraries.
I'm going to disable it for this API to unblock things/avoid a post-submit error.
There was a problem hiding this comment.
Do you think that the juice will be worth the squeeze vs. just disabling this warning across libraries ?
Now that you mention it: the improvements in the generated markdown are not going to be that much. OTOH, we may be able to clean up some code in sidekick.
You convinced me that we should start by disabling the warning in all clients (in case a new method or message gets a broken link). The annoying thing is that will generate changes for all clients, and therefore a large release.
There was a problem hiding this comment.
Filed several issues from this thread:
- adding
cargo doc -d warningto presubmit: ci: run cargo docs -D warnings as presubmit #5986 - disable bare_urls for all: all: disable doc rule bare_urls by default #5987
- add cargo doc to our onboarding workflow: rust: run cargo doc as part of onboarding workflow librarian#6608
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5970 +/- ##
=======================================
Coverage 97.71% 97.71%
=======================================
Files 242 242
Lines 60976 60976
=======================================
+ Hits 59581 59584 +3
+ Misses 1395 1392 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Towards googleapis/librarian#6594.