Prepare for multiple data-sources - #437
Conversation
53950f1 to
e3061b5
Compare
1ce96af to
b23489b
Compare
| // require("mocha-reporter").hook(); //Outputs filename in error logs with mocha-reporter | ||
|
|
||
| describe("Raw Events Integration", () => { | ||
| describe.skip("Raw Events Integration", () => { |
There was a problem hiding this comment.
I couldn't make the test work, because hardhat doesn't actually run an rpc server.
I think it's fine to disable it for now, since our integration tests do pretty much the same thing.
There was a problem hiding this comment.
Do we have an rpc integration test? Agree this test is fragile and I'd like to remove hardhat from our tests
There was a problem hiding this comment.
I think no. Need to make one of the integration tests to use RPC
| logger: Pino.t, | ||
| sources: array<Source.t>, | ||
| maxPartitionConcurrency: int, | ||
| mutable activeSource: Source.t, |
There was a problem hiding this comment.
Perhaps it makes sense to just track the index here and expose activeSource via public api function. That way it's easy to deal with incrementing/swapping out the source?
| let topics = switch topicSelection { | ||
| | {topic0, topic1: [], topic2: [], topic3: []} => [topic0] | ||
| | {topic0, topic1, topic2: [], topic3: []} => [topic0, topic1] | ||
| | {topic0, topic1: [], topic2, topic3: []} => [topic0, %raw(`null`), topic2] |
There was a problem hiding this comment.
Looks like empty topic 0 is not handled with the null case
There was a problem hiding this comment.
Perhaps there should be one more case with all of them empty
There was a problem hiding this comment.
I feel like there's a heck of a lot of cases to manually check like this. I kind of think pipelining it would be better. Like, map the topics into js nullable type and then pop off the null values from the end.
There was a problem hiding this comment.
We don't allow empty topic0
There was a problem hiding this comment.
I'd agree with you if we had nullable topic0, but in my opinion, having 8 cases in pattern matching is more straightforward than mapping & popping off trailing null values.
But I see your point. If you don't mind, I'll keep it like this, but the next time I touch the code, I'll change it to the solution you suggest.
There was a problem hiding this comment.
We don't allow empty
topic0
Ah yes, I forgot you can't wildcard without topic0
| envioVersion, | ||
| envioApiToken, | ||
| ecosystem: ecosystem.contents, | ||
| ecosystem: (config.ecosystem :> ecosystem), |
There was a problem hiding this comment.
I'm guessing the type system would shout if the @as field were different here? Any reason not to just use the same type here as the one in config?
| let sourceManager = SourceManager.make( | ||
| ~sources=[source], | ||
| ~maxPartitionConcurrency=3, | ||
| ~logger=Logging.logger, | ||
| ) |
There was a problem hiding this comment.
Maybe worth adding a mock function and calling at each of these places rather than inliniing all of them
JonoPrest
left a comment
JonoPrest
left a comment
There was a problem hiding this comment.
Cool Dmitry! Excited for the next changes 😄
2 participants
Initial data-structures clean up to allow multiple data sources.