Multi module configuration #494
Conversation
torkelrogstad
added
chain
In this commit we: 1) Introduce new database settings for all submodules, and at the same time remove the singular datbase setting for the entire project 2) Introduce a new method .initialize on all configs that is responsible for creating needed directories, databases and files 3) Introduce a new class BitcoinSAppConfig that wraps configs for all three other subproject config. We also provide implicit conversions that enable this super-config to be passed in wherever a specialized config is required. 4) Add more tests for our configuration setup.
torkelrogstad
force-pushed
the
2019-06-05-multi-module-conf
branch
from
2dfbbb1
to
7518769
Compare
| @@ -53,9 +58,14 @@ case class ChainAppConfig(val confs: Config*) extends AppConfig { | |||
| BlockHeaderDbHelper.fromBlockHeader(height = 0, | |||
| bh = | |||
| chain.genesisBlock.blockHeader) | |||
| val blockHeaderDAO = | |||
| BlockHeaderDAO()(ec = implicitly[ExecutionContext], appConfig = this) | |||
There was a problem hiding this comment.
why use implicitly? Can't we just use ec = ec?
There was a problem hiding this comment.
Just because I wasn't sure what the current execution context in scope was called. implictly works at compile level, so shoudln't make a difference. You could also argue that it's more readable, because it operates on the type level, and not on the "whatever I've called my constants"-level. If you disagree I can change it, not a big deal for me.
| * of this class can be passed in anywhere a wallet, | ||
| * chain or node config is required. | ||
| */ | ||
| case class BitcoinSAppConfig(confs: Config*) { |
There was a problem hiding this comment.
So what is the implication here if you are only using a portion of bitcoin-s (i.e. wallet with no node, or chain with no node etc)?
Maybe it doesn't matter and can be safely ignored?
I guess looking at this closer this is occurring in the testkit, so maybe it doesn't matter. But i would be nice to have something in src as well
There was a problem hiding this comment.
This is a very valid objection, but that's also why I placed it in testkit. Once we get some things going that are not just tests I think we can come back and revisit this point, and then we hopefully know more about what requirements and project layout looks like
|
It also looks like there are two bitcoind instances that are not being killed when tests run in this PR. EDIT: This is wrong, it is happening in master |
* Replace AppConfig with more specific types * Rework database configuration In this commit we: 1) Introduce new database settings for all submodules, and at the same time remove the singular datbase setting for the entire project 2) Introduce a new method .initialize on all configs that is responsible for creating needed directories, databases and files 3) Introduce a new class BitcoinSAppConfig that wraps configs for all three other subproject config. We also provide implicit conversions that enable this super-config to be passed in wherever a specialized config is required. 4) Add more tests for our configuration setup. * Add Ammonite to Docs deps
In this PR we:
and at the same time remove the singular datbase setting
for the entire project
.initialize()on all configsthat is responsible for creating needed directories,
databases and files
BitcoinSAppConfigthat wrapsconfigs for all three other subproject config. We
also provide implicit conversions that enable this
super-config to be passed in wherever a specialized
config is required.