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.

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

[Key Vault] Stop using the spread operator in the source files #9730

Closed
sadasant opened this issue Jun 26, 2020 · 2 comments · Fixed by #12697
Closed

[Key Vault] Stop using the spread operator in the source files #9730

sadasant opened this issue Jun 26, 2020 · 2 comments · Fixed by #12697
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. good first issue This issue tracks work that may be a good starting point for a first-time contributor KeyVault
Milestone

Comments

@sadasant
Copy link
Contributor

sadasant commented Jun 26, 2020

The spread operator produces inconsistent data in and out the convenience layer. We should be specific about what properties we pick to send and receive.

Also, change the variable interfaces from the public API into type structures with strict properties, so that the type system can catch this if we ever introduce the spread operator again.

@sadasant sadasant added this to the [2020] September milestone Jun 26, 2020
@sadasant sadasant self-assigned this Jun 26, 2020
@ramya-rao-a ramya-rao-a modified the milestones: [2020] September, Backlog Jun 26, 2020
@ramya-rao-a ramya-rao-a added the Client This issue points to a problem in the data-plane of the library. label Jun 26, 2020
@sadasant sadasant added the good first issue This issue tracks work that may be a good starting point for a first-time contributor label Oct 9, 2020
@sadasant sadasant modified the milestones: Backlog, MQ-2020 Oct 9, 2020
@sadasant
Copy link
Contributor Author

@ramya-rao-a I'd like to merge the full LRO refactoring first: #12630

That PR has some of the untangling required for this. For example: by separating the transformations into its own file in each client's source. After that PR, I'll be able to follow up more swiftly and close this issue with a simpler set of changes.

@sadasant
Copy link
Contributor Author

sadasant commented Nov 25, 2020

Some notes while I'm doing this:

  • I'm keeping all the spread operators in the generated folders.
  • I'm keeping all the spread operators on the arrays.
  • I'm keeping the request options related spread operators since the RequestOptions type allows any parameter to be sent through [key: string]: any;.
  • I'm keeping the tracing spread operator since tracing is already handled as cleanly as possible, I believe.
  • I'm keeping the spread operator on values of types that use RequireAtLeastOne.
  • I'm keeping the spread operators used in the pollers, since they're related to request options, which we receive from the user, and poller state, which we have tight control of. But feel free to challenge this!
  • I'm keeping the spread operators in the tests, since changing those won't affect the customers.
  • serviceVersion is being sent to InternalPipelineOptions, but InternalPipelineOptions doesn't have that property 🤔
  • loggingOptions doesn't have logPolicyOptions but we were sending this everywhere 🤔

@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. good first issue This issue tracks work that may be a good starting point for a first-time contributor KeyVault
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants