Add support for config file and refactor run config parsing#248
Add support for config file and refactor run config parsing#248BowTiedRadone wants to merge 8 commits intomasterfrom
Conversation
f5b1296 to
c359610
Compare
c359610 to
0eae34f
Compare
moodmosaic
left a comment
There was a problem hiding this comment.
Very nice refactoring 👍 A few comments below (and inline):
I think, a user typing:
rv ./project counter test --config=rv.config.json --seed=999
would have no indication that --seed=999 was discarded. Perhaps worth considering emitting a warning via the radio when both sources are present?
I also think that a typo like { "sedd": 42 } would be silently accepted as an empty config(?) If that's the case, consider warning on unrecognized keys.
| manifestDir, | ||
| sutContractName, | ||
| type: normalizedType as "test" | "invariant", | ||
| seed: options.seed ? parseInt(options.seed, 10) : undefined, |
There was a problem hiding this comment.
If a user passes --seed=abc, parseInt("abc", 10) produces NaN, which will silently propagate into fast-check as the seed. The config-file path validates this properly ("seed" must be an integer), so there's an asymmetry.
| } | ||
|
|
||
| // All addresses includes simnet addresses plus any new config addresses. | ||
| const allAddresses = [...merged.values()]; |
There was a problem hiding this comment.
This now excluded faucet, right? If this is intentional, a comment would help(?)
|
|
||
| Learn more: https://stacks-network.github.io/rendezvous/ | ||
| `; | ||
| const parseCliOrExit = (argv: string[], radio: EventEmitter) => { |
There was a problem hiding this comment.
The catch branch always calls process.exit(1) (which returns never). An explicit annotation would make the contract clearer for readers
| const parseCliOrExit = (argv: string[], radio: EventEmitter) => { | |
| const parseCliOrExit = (argv: string[], radio: EventEmitter): RunConfig | undefined => { |
(Might have to be formatted to fit the 79-char width.)
Co-authored-by: Nikos Baxevanis <nikos.baxevanis@gmail.com>
|
@moodmosaic Thanks for the review. All should be addressed now |
This PR:
mainto keep the entry point clean--configflag for file-based configurationFixes #183.