-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: fixes the filtering logic #110
Conversation
func (f *HiddenEnvsFilter) removeContentIfHiddenForEnv(contentType *openapi3.MediaType) { | ||
if isContentTypeHiddenForEnv := isContentTypeHiddenForEnv(contentType, f.metadata.targetEnv); isContentTypeHiddenForEnv { | ||
log.Printf("Removing contentType: %q because is hidden for target env: %q", contentType.Schema.Ref, f.metadata.targetEnv) | ||
contentType.Schema = nil // Remove ContentType if it is hidden for the target environment |
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.
contentType.Schema = nil // Remove ContentType if it is hidden for the target environment
This piece of code was removing the schema instead of the ContentType
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.
but why are we breaking the patterns you stablished in this filter with the f.removeContentIfHiddenForEnv
?
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.
The main reason was that we need to pass the k
and response.Value.Content
as we need them to delete the content.
func ApplyFilters(doc *openapi3.T, metadata *Metadata) error { | ||
if err := initFilters(doc, metadata); err != nil { | ||
return err | ||
func ApplyFiltersWithInit(doc *openapi3.T, metadata *Metadata, init func(oas *openapi3.T, metadata *Metadata) []Filter) (*openapi3.T, error) { |
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.
[nit] I think naming this WithInit
leaks filter.go logic that is not necessary. Would make more sense to call this ApplyFilter and pass the filter type, maybe? and do the init logic here instead of outside
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.
do you think the problem is the name and/or the fact that we are passing a function returning an array of filters?
I can call the function newFilters
if we think init
is the problem
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.
Updated. Let me know what you think
func (f *HiddenEnvsFilter) removeContentIfHiddenForEnv(contentType *openapi3.MediaType) { | ||
if isContentTypeHiddenForEnv := isContentTypeHiddenForEnv(contentType, f.metadata.targetEnv); isContentTypeHiddenForEnv { | ||
log.Printf("Removing contentType: %q because is hidden for target env: %q", contentType.Schema.Ref, f.metadata.targetEnv) | ||
contentType.Schema = nil // Remove ContentType if it is hidden for the target environment |
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.
but why are we breaking the patterns you stablished in this filter with the f.removeContentIfHiddenForEnv
?
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 thanks for the changes!
Proposed changes
The PR proposes the following changes: