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

Removes more timeout from storage operation builders #919

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

bmc-msft
Copy link
Contributor

@bmc-msft bmc-msft commented Jul 11, 2022

With multiple PRs landing one on top of each other, not all of the timeouts as part of the bulider patterns were removed during #893.

These remove all but two that use the Storage crate, both in data_tables.

  1. TransactionBuilder. This Builder does not use setters, but instead provides it's own builder methods to enable adding multiple database operations into the single transaction. It does not need the setters portion of the operation macro, but should still be a builder.
  2. UpdateOrMergeEntityBuilder. This no longer needs to be a builder, as the only optional field is now handled via aforementioned PR.

In both of these cases, removing the timeout option from the operation causes the macro to fail, as it requires at least one optional parameter.

With TransactionBuilder, I'd prefer the macro be updated to support no optional params if we're providing our own setters.

With UpdateOrMergeEntityBuilder, this probably doesn't need to be an operation, however it's the only operation that doesn't need .into_future, which makes it feel weird from a user's perspective. Once IntoFuture is stable, this could just return a future directly without using the builder pattern.

With multiple PRs landing one on top of each other, not all of the
timeouts as part of the bulider patterns were removed during Azure#893.

These remove all but two that use the Storage crate, both in `data_tables`.

1. TransactionBuilder.  This Builder does not use `setters`, but instead
   provides it's own builder methods to enable adding multiple database
   operations into the single transaction.  It does not need the `setters`
   portion of the operation macro, but should still be a builder.
2. UpdateOrMergeEntityBuilder.  This no longer needs to be a builder, as
   the only optional field is now handled via aforementioned PR.
@bmc-msft bmc-msft requested a review from rylev July 11, 2022 13:49
Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this merged, but I have a note about your description of the issue:

In both of these cases, removing the timeout option from the operation causes the macro to fail, as it requires at least one optional parameter.

This is not true. For example:

operation! {
CreateTable,
client: TableClient,
}

@rylev rylev merged commit b794fc9 into Azure:main Jul 11, 2022
@bmc-msft bmc-msft deleted the remove-timeout-from-storage-operations branch July 11, 2022 15:20
@bmc-msft
Copy link
Contributor Author

@rylev , I see what lead me to make that mistaken assessment.

The last optional field must not have a comma, while the last required field must have a comma.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants