-
Notifications
You must be signed in to change notification settings - Fork 8
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
(VDB-1046) Add command to extract diffs #27
Conversation
- Isolate diff extraction from transformers etc
744bd8e
to
bc2ea7d
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.
Looks great, solid refactoring!
@@ -20,9 +20,9 @@ import ( | |||
"math/rand" | |||
|
|||
"github.com/ethereum/go-ethereum/common" | |||
storage_factory "github.com/makerdao/vulcanizedb/libraries/shared/factories/storage" | |||
"github.com/makerdao/vulcanizedb/libraries/shared/factories/storage" |
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.
🎉
err := extractor.ExtractDiffs() | ||
|
||
Expect(err).To(HaveOccurred()) | ||
Expect(err).To(MatchError(fakes.FakeError)) |
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.
Tiny nitpick, but do we need both of these assertions?
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.
We probably don't need both, but I'm a fan because of the clearer failure message (expected err to have occurred, got none
vs expected nil to equal "Failed"
). Not super wedded to this, but I think I'd opt for a more comprehensive approach if we decide to minimize our assertions throughout the test suites
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.
Nice, yeah I agree that isn't a very good test failure if the error didn't occur. Especially if we're using this pattern elsewhere I def have no problem leaving it like this
} | ||
|
||
type StorageQueue struct { | ||
type queue struct { |
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.
💡
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.
this looks awesome, super excited to get this included 🦖
Notes:
storage
package to avoid import cycles (e.g.datastore
importingstorage
and vice versa). Not a huge fan of having a package namedtypes
since it overloads geth, but also not wanting to go down too deep a rabbit hole until we're restructuring things more broadly.staging
.