enhancement: plugin interface #83
Conversation
Codecov Report
@@ Coverage Diff @@
## master #83 +/- ##
==========================================
+ Coverage 67.66% 69.54% +1.88%
==========================================
Files 32 36 +4
Lines 1976 2417 +441
==========================================
+ Hits 1337 1681 +344
- Misses 570 645 +75
- Partials 69 91 +22
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
tzaffi
changed the title
| if err != nil { | ||
| return fmt.Errorf("Pipeline.Init(): could not initialize importer (%s): %w", p.cfg.Importer.Name, err) | ||
| } | ||
| genesis, err := (*p.importer).GetGenesis() |
There was a problem hiding this comment.
new interface method in use
| @@ -55,6 +54,7 @@ type algodImporter struct { | |||
| ctx context.Context | |||
| cancel context.CancelFunc | |||
| mode int | |||
| genesis *sdk.Genesis | |||
There was a problem hiding this comment.
Cache the genesis in the algodImporter struct to avoid having to call the genesis endpoint after Init()
| @@ -149,7 +151,8 @@ func TestInitCatchup(t *testing.T) { | |||
| targetRound sdk.Round | |||
| adminToken string // to trigger fast-catchup | |||
| algodServer *httptest.Server | |||
| err string | |||
| errInit string | |||
| errGetGen string | |||
There was a problem hiding this comment.
It looks like you didn't end up using this?
There was a problem hiding this comment.
See for example lines 195/196. Sometimes I omitted them when they were "", but if that's confusing I can add these back in.
There was a problem hiding this comment.
It looks like 195 is also ""
There was a problem hiding this comment.
The latest commit adds in all zero-valued errInit and errGetGen:
There was a problem hiding this comment.
I mean that none of these tests define errGetGen to anything besides an empty string, so it could be removed.
There was a problem hiding this comment.
Thanks for these observations. My gut was telling me that we need a failure case for full coverage and I half-baked a solution by providing errGetGen. But both of you observed that I didn't actually finish baking the cake. Now it should be all baked: 5b114a0
Re-organize Plugin Interfaces
These changes grew out a personal project to create a binary conduit plugin (or plugins) which will help me calculate taxes. As I go along and spot various opportunites for streamlining, clarifying the requirements, and docs, I'll continue putting up small[1] PR's of this kind.
Except for the last commit (see Odd Finding below), all changes are non-functional. I'm expecting to roll back the last commit, but it's worth further discussion.
Summary of Changes
.markdownlint.yml. This isn't enforced in CI but while using the VS Code extension it helped me identify some bugs in our docs. For example, some improperly formatted list items.Plugin interfaceextendingPluginMetadataand with methodsConfig()andClose()inconduit/plugins/plugin.go. These are extended by theImporter,ProcessorandExporterinterfaces instead ofPlugingMetadataresulting in changes in:conduit/plugins/exporters/exporter.goconduit/plugins/importers/importer.goconduit/plugins/processors/processor.godocs/Configuration.mddocs/GettingStarted.mddocs/tutorials/WritingBlocksToFile.mddocs/tutorials/IndexerMigration.mdgenesisvalue returned from newly introducedImporter.GetGenesis()meets expectations in various unit tests.Odd Finding
Notice that all tests pass after I commented out all usages of
Config(). Is this actually used anywhere? If not, we should consider removing it from the plugin interfaces.Upshot of Odd Finding
After a discussion, we decided to streamline the
Plugininterface as follows:Config()from the interfacesPluginMetadatadirectly intoPlugininterfaceInit()toPlugininterface and...GetGenesis()fromImporter.Init()[1]: It was still small at the original time that I created this PR description.