[Feat] Added New AccuWeather Detector Version #4114
Conversation
| _ detectors.Detector = (*Scanner)(nil) | ||
|
|
||
| // Make sure that your group is surrounded in boundary characters such as below to reduce false positives. | ||
| keyPat = regexp.MustCompile(detectors.PrefixRegex([]string{"accuweather"}) + `\b([a-zA-Z0-9]{32})\b`) |
There was a problem hiding this comment.
In my opinion, the pattern difference between V1 and V2 isn’t significant, and both can be validated against the same API. Could we loosen the V1 pattern slightly to accommodate V2 as well?
There was a problem hiding this comment.
Hey @kashifkhan0771. I believe the pattern would become \([a-z0-9A-Z\%]{35})|\b[a-zA-Z0-9]{32})\b, correct?
Other than avoiding the extra computational cost of the regex, I believe having a newer version of the detector may be make it simpler for us to end support for the older version in case we need to.
There's other examples of new versions being added solely to accommodate a pattern difference such as the netlify v1 and v2 detectors.
What are your thoughts on this?
| @@ -13,7 +13,7 @@ import ( | |||
| ) | |||
|
|
|||
| type Scanner struct { | |||
| client *http.Client | |||
| Client *http.Client | |||
There was a problem hiding this comment.
I might have overseen but I could not understand to make it public ?
There was a problem hiding this comment.
The v2 integration tests include a test cases that require Client to be exported, e.g.:
| matches[k] = struct{}{} | ||
| } | ||
| allMatches := keyPat.FindAllStringSubmatch(string(data), -1) | ||
| return s.VerifyAllMatches(ctx, allMatches, verify) |
There was a problem hiding this comment.
Can we call it processMatches ? I believe this function is processing then verifying the findings.
There was a problem hiding this comment.
Yes, thanks. That does seem like a more suitable name. Since it needs to be exported, I can rename it to ProcessMatches.
Description:
Added a new AccuWeather API detector version to support an updated pattern of keys
Checklist:
make test-community)?make lintthis requires golangci-lint)?