Initial correctifier code. #164
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
==========================================
- Coverage 51.66% 50.10% -1.57%
==========================================
Files 31 34 +3
Lines 4746 4894 +148
==========================================
Hits 2452 2452
- Misses 2104 2252 +148
Partials 190 190
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| ) | ||
|
|
||
| // Correctifier applies a series of normalizing options to a WRP message. | ||
| type Correctifier struct { |
There was a problem hiding this comment.
This type stutters: wrpcorrectify.Correctifier. The usual way to do this in golang is to call this type T: wrpcorrectify.T. The idea is that it's a central type to the package. I'm dubious about this, but it is what's done.
Probably a better way is to rename either the type or the package to something that doesn't stutter.
There was a problem hiding this comment.
Yeah, these were just working names.
| // Option is a functional option for normalizing a WRP message. | ||
| type Option interface { | ||
| // Correctify applies the option to the given message. | ||
| Correctify(context.Context, *wrp.Message) error |
There was a problem hiding this comment.
Is the intention to allow code outside this package to create options? If so, some additional bulletproofing might be in order. However, if no code outside this package should be creating Option implementations, then this interface should be sealed: no exported methods.
There was a problem hiding this comment.
I was thinking that by making the Option something outside code could do, it would make the functionality more extensible & useful for one-off places.
What kind of bulletproofing did you have in mind?
|
|
||
| // CounterMetric provides a counter metric that can be used to track the number | ||
| // of times a specific error occurs. | ||
| type CounterMetric struct { |
There was a problem hiding this comment.
This code might benefit from inverting the metric function. You can avoid any dependency on prometheus. Instead, expose methods that return whatever state you want to track, then require application code to use the *Func versions of metrics. For example: https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#NewCounterFunc.
There was a problem hiding this comment.
I like your suggestion better.
| // ReplaceAnySelfLocator replaces any `self:` based locator with the scheme and | ||
| // authority of the given locator. If the given locator is not valid, the | ||
| // option returns an error. | ||
| func ReplaceAnySelfLocator(me string) Option { |
There was a problem hiding this comment.
You might be able to cut down the code repetition by have fewer or just one option that was parameterized. For example:
func ReplaceAnyLocator(scheme string, me string) Option { ...}
You could also use struct fields via the reflect package to bind together a replacement value with the wrp.Message fields it applies to.
|
Closing in favor of #167 |
This is a code doodle that shows what I'm thinking for the options based validator & normalizer code. I think this should support the metrics use cases we have & probably more if we want.
I would like to chat about this tomorrow if possible.