Conversation
antkmsft
requested review from
RickWinter,
heaths and
jhendrixMSFT
as code owners
There was a problem hiding this comment.
Pull request overview
Adds a new Spector Azure client-generator-core test crate to exercise @clientDefaultValue behavior in the Rust emitter, including generated client/models and integration tests, and wires it into the test workspace + TypeSpec compilation script.
Changes:
- Introduces new
spector_client_default_valuecrate with generated client and models for default-value scenarios. - Adds Tokio integration tests for model/property, query/header, and path parameter defaulting scenarios.
- Registers the new crate in
packages/typespec-rust/testworkspace and in.scripts/tspcompile.js.
Reviewed changes
Copilot reviewed 5 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/typespec-rust/test/spector/azure/client-generator-core/client-default-value/tests/client_default_value_client_test.rs | New integration tests for default-value behavior. |
| packages/typespec-rust/test/spector/azure/client-generator-core/client-default-value/src/lib.rs | Crate entrypoint re-exporting generated code. |
| packages/typespec-rust/test/spector/azure/client-generator-core/client-default-value/src/generated/models/models.rs | Generated model with documented client default values. |
| packages/typespec-rust/test/spector/azure/client-generator-core/client-default-value/src/generated/models/models_impl.rs | Generated TryFrom impl for request content serialization. |
| packages/typespec-rust/test/spector/azure/client-generator-core/client-default-value/src/generated/models/mod.rs | Models module wiring and exports. |
| packages/typespec-rust/test/spector/azure/client-generator-core/client-default-value/src/generated/models/method_options.rs | Generated per-method options structs. |
| packages/typespec-rust/test/spector/azure/client-generator-core/client-default-value/src/generated/mod.rs | Top-level generated module wiring. |
| packages/typespec-rust/test/spector/azure/client-generator-core/client-default-value/src/generated/clients/mod.rs | Clients module wiring and exports. |
| packages/typespec-rust/test/spector/azure/client-generator-core/client-default-value/src/generated/clients/client_default_value_client.rs | Generated client implementation for the default-value endpoints. |
| packages/typespec-rust/test/spector/azure/client-generator-core/client-default-value/Cargo.toml | New crate manifest for the Spector test crate. |
| packages/typespec-rust/test/Cargo.toml | Adds the new crate to the test workspace members. |
| packages/typespec-rust/test/Cargo.lock | Adds the new crate to the lockfile. |
| packages/typespec-rust/.scripts/tspcompile.js | Adds the new crate/spec entry to the Azure HTTP specs generation group. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
...r/azure/client-generator-core/client-default-value/tests/client_default_value_client_test.rs
Show resolved
Hide resolved
...r/azure/client-generator-core/client-default-value/tests/client_default_value_client_test.rs
Show resolved
Hide resolved
...r/azure/client-generator-core/client-default-value/tests/client_default_value_client_test.rs
Show resolved
Hide resolved
...r/azure/client-generator-core/client-default-value/tests/client_default_value_client_test.rs
Show resolved
Hide resolved
| 'spector_apiverheader': {input: 'azure/client-generator-core/api-version/header/client.tsp'}, | ||
| 'spector_apiverpath': {input: 'azure/client-generator-core/api-version/path/client.tsp'}, | ||
| 'spector_apiverquery': {input: 'azure/client-generator-core/api-version/query/client.tsp'}, | ||
| 'spector_client_default_value': {input: 'azure/client-generator-core/client-default-value'}, |
There was a problem hiding this comment.
The decorator is part of the legacy namespace Azure.ClientGenerator.Core.Legacy.clientDefaultValue. Do we want/need to support this?
There was a problem hiding this comment.
That is the question I am asking in this PR. Do you think we should? I will also ask @heaths.
In the current state of the PR, we won't support it, because not implementing it is cheaper than implementing and throwing it away, and my thought was also that we maight want to not support it. But that can be done, relatively inexpensively, I think. We would emit a couple of impl Default for ...s.
I'd only say, if not supporting it is the intended behavior, there is no reason to not report it to Spector, as if we have a problem, as if something yet needs to be covered. If we think that we're, at least currently, view it as that we don't intend to support that, let's still run the tests to keep them green. The tests will formalize the semantics, and will state that it is intentional.
If we change this decision in the future, we can update the emitter code, and also update the tests.
From the prespective of not having breaking changes in the future, it seems ok to me to not implement it now if we're not sure.
There was a problem hiding this comment.
For model properties it's a pretty easy change and there's no compat problems.
The interesting case is for method parameters. If a required method param has a client-side default value, then it becomes optional which would clearly be a breaking change.
If the scope is models only then I agree that we don't have to decide now. Are there any params?
There was a problem hiding this comment.
Per #898, this PR should implement Default for options and models, and as for the function parameter, it would move it to options (and implement Default the same way). Sounds good.
Please let me know if that is not what you'd like to see.
I will prioritize updating this PR after the current release blockers are fixed.
See the "
// Supposed to be the default value" comment in the test (5 places).Defaultfor options? (4/5 places)"default-segment1"one). I guess we could move the first parameter into Options, and then implementDefault...