-
Notifications
You must be signed in to change notification settings - Fork 106
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
feat(recipes): add exclusion patterns when wildcard is used #372
Conversation
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.
Overall looks good but a small suggestion to keep things simple; happy to discuss!
@@ -228,15 +254,29 @@ func (classifier *Classifier) Classify(data detections.Detection) (*ClassifiedIn | |||
}, nil | |||
} | |||
|
|||
func (classifier *Classifier) FindMatchingRecipeUrl(detectionURL string) (*RecipeURLMatch, error) { | |||
func (classifier *Classifier) FindMatchingRecipeUrl(detectionURL string) (*RecipeURLMatch, *classify.ClassificationDecision, 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.
Ideally this func wouldn't care about classification and would only care about matching URLs.
Suggestion to add an ignored or excluded Boolean
to RecipeURLMatch
, which we can then use to classify the URL in the calling function above, and simplify the concerns of this func. What do you think?
@@ -92,6 +93,20 @@ func New(config Config) (*Classifier, error) { | |||
} | |||
preparedRecipe.URLS = append(preparedRecipe.URLS, preparedRecipeURL) | |||
} | |||
|
|||
for _, ignoredRecipeURL := range recipe.ExcludeURLS { |
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 think we should pick either ignored or excluded and not mix the two terms.
for _, ignoredRecipeURL := range recipe.ExcludeURLS { | |
for _, excludedRecipeURL := range recipe.ExcludeURLS { |
@elsapet I agree. Updated the PR following your recommendations. Let me know what you think :) |
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.
😍 👏
|
||
if match != "" { | ||
return &RecipeURLMatch{ | ||
ExcludedURL: true, |
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.
Since we have this info, we could include it (may be useful for debugging). I'll leave it up to you!
ExcludedURL: true, | |
ExcludedURL: true, | |
DetectionURLPart: match, | |
RecipeURL: recipeURL.URL, | |
RecipeUUID: recipe.UUID, | |
RecipeName: recipe.Name, | |
RecipeSubType: recipe.SubType, |
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.
Not sure yet
I'll merge it like this let's see how useful it is :)
Description
closes #370
Checklist