-
Notifications
You must be signed in to change notification settings - Fork 14
CLOUDP-292068: Remove x-xgen-IPA-exception from version split OASes #351
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
Conversation
|
||
// Filter: ExtensionFilter is a filter that updates the x-sunset and x-xgen-version extensions to a date string | ||
// and deletes the x-sunset extension if the latest matched version is deprecated by hidden versions | ||
// Filter: ExtensionFilter is a filter that deleted the x-xgen-ipa-exception extensions, updates the x-sunset and x-xgen-version |
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.
// Filter: ExtensionFilter is a filter that deleted the x-xgen-ipa-exception extensions, updates the x-sunset and x-xgen-version | |
// ExtensionFilter is a filter that deleted the x-xgen-ipa-exception extensions, updates the x-sunset and x-xgen-version |
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.
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.
cc @blva godocs should start with the method name any reason we don't follow that rule here?
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.
I just followed a format I found in oasdiff to update the docs: https://github.com/mongodb/openapi/blob/main/tools/cli/scripts/doc_filters.sh
This can be fixed in a separate JIRA / PR if we want to udpate it everywhere, I think it's easy to update but I don't have the bandwidth right now
updateExtensionToDateString(operation.Extensions) | ||
deleteIpaExceptionExtension(operation.Extensions) | ||
|
||
for _, parameter := range operation.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.
🐻 with me this is a bit messy but need to look at the code in an editor to help you a bit
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.
Yup it's a bit messy, happy to get feedback on this one 😅
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.
Would it make sense to break the code into separate functions, with each function handling a specific component type? It might make the code easier to follow and maintain
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.
@yelizhenden-mdb 💯 yes that was one approach but I think filtering in general needs some love
I haven't really have the time to go deeper here since I found other stuff in the repo that needed fixing like some improved linting but you can split this in functions and leverage the fact that go encourage argument modification, is quite common in go to name an argument out
to flag it will be modified by the function
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.
Split some parts to more functions, let me know if it's a bit better.
is quite common in go to name an argument out to flag it will be modified by the function
Not sure what you mean here, unless you mean naming the parameter to out
} | ||
} | ||
|
||
func TestExtensionFilter_removeIpaException(t *testing.T) { |
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.
some of these seem like test cases see for how we do test cases in go
openapi/tools/cli/internal/apiversion/version_test.go
Lines 25 to 30 in e1a969d
testCases := []struct { | |
name string | |
contentType string | |
expectedMatch string | |
wantErr bool | |
}{ |
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.
LGTM
Proposed changes
Updates the FOAS CLI to remove the x-xgen-ipa-exception extensions from the version split oases.
Jira ticket: CLOUDP-292068
Checklist