cnct: add resolver constructors#3665
Conversation
There was a problem hiding this comment.
What do these methods really achieve, that Decode doesn't?
There was a problem hiding this comment.
The end result is the same, but it encapsulates the instantiation of the resolver inside the resolver. If we want to follow up with additional initialization of resolvers (for example set up the initial report), it is clear where to do it. Otherwise that init code will need to be called separately from chan arb which carries more risk of making a mistake.
joostjager
force-pushed
the
resolver-constructors
branch
2 times, most recently
from
3658a7f to
219f047
Compare
|
Now the resolvers are really ready to get their reports initialized in their (new or decode) constructors |
joostjager
force-pushed
the
resolver-constructors
branch
from
219f047 to
ee35bed
Compare
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
I'm struggling to review this one meaningfully, I don't quite have enough context. Added two nits for now, and I'll try another pass when I understand it more.
joostjager
force-pushed
the
resolver-constructors
branch
2 times, most recently
from
82c66ff to
9a4aced
Compare
Removes a bug-prone construction that existed previously where the quit channel was replaced just-in-time.
joostjager
force-pushed
the
resolver-constructors
branch
from
9a4aced to
d9e25f2
Compare
There was a problem hiding this comment.
I don't really see the point of splitting these into different methods instead of using the interface Decode method, now that they all have the same method signature.
There was a problem hiding this comment.
Renamed to newResolverFromReader. Resolver structs aren't dumb data structs that can just have a Serializable interfaces. There is more logic required, which is probably also why the Attach and Supplement method appeared.
joostjager
force-pushed
the
resolver-constructors
branch
from
d9e25f2 to
a8546c4
Compare
joostjager
force-pushed
the
resolver-constructors
branch
from
a8546c4 to
32249cb
Compare
| return c, nil | ||
| } | ||
|
|
||
| // AttachConfig should be called once a resolved is successfully decoded from |
There was a problem hiding this comment.
nit: delete in the same commit as we remove the last reference to this method.
There was a problem hiding this comment.
This is the last commit that contains the last ref?
3 participants
This PR makes the instantiation of resolvers stricter and thereby opens up the path to also add initialization logic to a resolver.
Spin off of #3644