-
Notifications
You must be signed in to change notification settings - Fork 17
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: update event types to be discreet #321
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.
In the broadest sense, loooking good -- direction is strong.
The only suggestion is to switch to a type switch in aggregateeventrecorder.
I'm also not convinced about payloadcid, unless we really have a value everywhere.
Otherwise, I like these changes a lot.
441d39b
to
79307ac
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #321 +/- ##
==========================================
+ Coverage 75.65% 76.60% +0.94%
==========================================
Files 70 85 +15
Lines 6310 6403 +93
==========================================
+ Hits 4774 4905 +131
+ Misses 1262 1226 -36
+ Partials 274 272 -2
|
cdc0bf2
to
48d4d43
Compare
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 only concerning thing to me is we appear to be qualitatively changing the behavior for server timings in the IPFS handler and I want to make sure @willscott is ok with that.
Otherwise, I'm good. A few small nits. Otherwise, feel free to merge tomorrow once @rvagg has reviewed.
48d4d43
to
bc8b309
Compare
bc8b309
to
2879b02
Compare
Hi! I'm still holding @willscott 's review of the changes to serving timings cause those are used elsewhere in the stack. I've ping him on slack for visibility. |
2879b02
to
81a2850
Compare
Everything has been resolved or moved to another issue. Waiting on PR #332 before merging this. |
In an effort to cut down on the unneccessary bloat some of our events started to have, this update removes the idea of event phases in favor of making the events discreet from each other. Events should not depend on each other and should not require data that is not relative to them.
e51af12
to
299289d
Compare
In an effort to cut down on the unneccessary bloat some of our events started to have, this update removes the idea of event phases in favor of making the events discreet from each other. Events should not depend on each other and should not require data that is not relative to them.
Notable Changes
Note: fetch in this case is the beginning of
Lassie.Fetch()
and retrieval is when we start talking to a specific provider.EventWithSPID
. Checks have been added in places to ensure the event has a given property before using.I attempted to extract payloadCid out of the base event, but got caught on some assignable candidate finder tests that use CID keyed maps to assert that specific candidates were found for a given CID. The entire test set would have needed to be rewritten. I've made those changes to the
feat/remove-cid-from-events
branch if we wanted to move forward with this work. I'm going to leave the payloadCid in the base event for this PR.