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

ARM: Add rule to enable disabling DeleteResponseCodes LintDiff rule #298

Merged
merged 9 commits into from
Mar 1, 2024

Conversation

tjprescott
Copy link
Member

@tjprescott tjprescott commented Feb 22, 2024

@azure-sdk
Copy link
Collaborator

azure-sdk commented Feb 22, 2024

All changed packages have been documented.

@tjprescott tjprescott force-pushed the deleteResponseCodesRule branch 4 times, most recently from f636103 to f745247 Compare February 23, 2024 15:30
@azure-sdk
Copy link
Collaborator

@tjprescott tjprescott force-pushed the deleteResponseCodesRule branch from 1ac4c06 to ead0b93 Compare February 23, 2024 16:07
@tjprescott tjprescott marked this pull request as ready for review February 23, 2024 16:28
@tjprescott tjprescott force-pushed the deleteResponseCodesRule branch 3 times, most recently from b31e4b7 to 3f61976 Compare February 27, 2024 16:53
Copy link
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

Looks good. A coupl eof comments.

Also, we should deprecate ArmResourceDeleteAsync unfortunately, this will likely mean adding some deprecation suppressions in samples.

@tjprescott
Copy link
Member Author

tjprescott commented Feb 28, 2024

@markcowl updated. Please take a look at the splash radius associated with deprecating ArmResourceDeleteAsync.

It seems pretty high, at least in the tests. Hopefully real specs aren't prolifically using this bad pattern 😨

@tjprescott tjprescott force-pushed the deleteResponseCodesRule branch 3 times, most recently from faf1f6f to 687a77c Compare February 29, 2024 20:01
@tjprescott tjprescott force-pushed the deleteResponseCodesRule branch 2 times, most recently from b1bf769 to 1a67086 Compare February 29, 2024 20:42
@tjprescott
Copy link
Member Author

I will rebase this once approved.

Copy link
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

It looks like some changes might be missing from the PR. Comments point these out

@tjprescott tjprescott force-pushed the deleteResponseCodesRule branch 3 times, most recently from 949eb65 to cc9b5e3 Compare March 1, 2024 16:09
@tjprescott tjprescott force-pushed the deleteResponseCodesRule branch from cc9b5e3 to b9aa706 Compare March 1, 2024 16:38
@@ -180,6 +180,7 @@ model FlangeProperties {
...DefaultProvisioningStateProperty;
}

#suppress "deprecated" "Should replace TenantResourceDelete in the future"
Copy link
Member Author

Choose a reason for hiding this comment

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

@markcowl I was able to update the other samples to simply use ArmResourceDeleteWithoutOkAsync but this is an interface template of a template of a template, etc. so I just suppressed the deprecation warning...

@@ -412,6 +412,7 @@ op ArmResourceDeleteAsyncBase<
* @template Response Optional. The success response(s) for the delete operation
* @template Error Optional. The error response, if non-standard.
*/
#deprecated "Use 'ArmResourceDeleteWithoutOkAsync' instead"
Copy link
Member Author

Choose a reason for hiding this comment

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

You can see the cascading effects of formally deprecating this.

@tjprescott
Copy link
Member Author

@markcowl all comments should be resolved. The REST API specs PR has been merged

@allenjzhang allenjzhang merged commit 63fa13d into main Mar 1, 2024
14 checks passed
@allenjzhang allenjzhang deleted the deleteResponseCodesRule branch March 1, 2024 19:29
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.

5 participants