-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add new rule: 1050 - ParameterLocationHasChanged
; Change behavior of ChangedParameterOrder
rule with regards to x-ms-parameter-location
#334
Conversation
ParameterOrderChanged
with regards to x-ms-parameter-location
ParameterOrderChanged
with regards to x-ms-parameter-location
ChangedParameterOrder
rule with regards to x-ms-parameter-location
openapi-diff/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerCompareTests.cs
Show resolved
Hide resolved
fe0aeaa
to
6914784
Compare
{ | ||
var curOriginalParameter = Parameters.ElementAt(i); | ||
var curParameter = currentOperationParameters.ElementAt(i); | ||
curParameter.Extensions.TryGetValue("x-ms-long-running-operation", out var curParameterLocation); |
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.
FYI @mikekistler @mikekistler there was a bug here for over 2 years (#218) where this:
curParameter.Extensions.TryGetValue("x-ms-long-running-operation", out var curParameterLocation);
should have been:
curParameter.Extensions.TryGetValue("x-ms-parameter-location", out var curParameterLocation);
But that is not all. There also was a bug where parameters whose order doesn't matter would cause false positives on parameters whose order matters but didn't actually change. That latter bug was just hidden by the former bug. I fixed all of that in this PR
@mikekistler can you review and approve? |
ChangedParameterOrder
rule with regards to x-ms-parameter-location
1050 - ParameterLocationHasChanged
; Change behavior of ChangedParameterOrder
rule with regards to x-ms-parameter-location
@mikekistler I applied the changes per our chat. Please see the diff for all the changes. Notably: |
"produces": [ | ||
"text/plain" | ||
], | ||
"parameters": [ |
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.
@mikekistler per the ParameterRemoved
test I added, here the removal of the params named "path_param", "path_param_req", "query_param_req", "body_param_req"
will result in RemovedRequiredParameter
breaking change each, while removal of "body_param", "query_param"
will result in RemovedOptionalParameter
breaking change.
Notably:
- no
ChangedParameterOrder
breaking change will be reported. path_param
is consideredrequired
even though the param itself is not marked as such.
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.
Looks good! 👍
Pull Request 559774: Update |
Addresses: