-
Notifications
You must be signed in to change notification settings - Fork 6
Add coverage for client default values #895
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| [package] | ||
| name = "spector_client_default_value" | ||
| version = "0.1.0" | ||
| authors.workspace = true | ||
| edition.workspace = true | ||
| license.workspace = true | ||
| repository.workspace = true | ||
| rust-version.workspace = true | ||
|
|
||
| [features] | ||
| default = ["azure_core/default"] | ||
|
|
||
| [dependencies] | ||
| azure_core = { workspace = true } | ||
| serde = { workspace = true } | ||
|
|
||
| [dev-dependencies] | ||
| tokio = { workspace = true } |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking issue #898
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.
Per #898, this PR should implement
Defaultfor options and models, and as for the function parameter, it would move it to options (and implementDefaultthe 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.