feat: Extend support for env-based configuration #631
Conversation
BREAKING CHANGE: `Config::from_env` now returns an `eyre::Result`
pierre-l
force-pushed
the
config-from-env
branch
from
35d374e
to
c030b7d
Compare
pierre-l
force-pushed
the
config-from-env
branch
from
a6c059d
to
228df01
Compare
|
@sergey-melnychuk @LKozlowski I added a second commit for the unit tests |
| }; | ||
|
|
||
| Ok(Self { | ||
| network: Network::from_str(&get("NETWORK").unwrap_or_default()) |
There was a problem hiding this comment.
if the provided network is invalid, then mainnet is used as a default which might not be always desired behavior; I think an error must be returned when provided network is invalid - with respective test case.
| eth_execution_rpc: require("ETH_EXECUTION_RPC")?, | ||
| starknet_rpc: require("STARKNET_RPC")?, | ||
| data_dir: PathBuf::from(get("DATA_DIR").unwrap_or_default()), | ||
| poll_secs: u64::from_str(&get("POLL_SECS").unwrap_or_default()) |
There was a problem hiding this comment.
if provided poll seconds value is invalid also the default is used - probably safer to return an error; will also require one more test case for that
There was a problem hiding this comment.
This is the original behavior. It's out of scope of this PR. It doesn't help with with my original task. It would be another breaking change.
|
I really wish is was possible to fix one little thing without having to fix everything that's around, significantly extending the scope of an initially trivial PR, and bringing multiple breaking changes in the process. #630 will use config files. |
I guess I am sometimes getting carried away with "ownership mentality" over the project and the boy scout rule ("Leave your code better than you found it.") Yet I personally believe there is still value to improve the code at every opportunity provided. If the original behavior allows invalid configuration parameters to be propagated through the system, I don't see why we should stick to it and not improve it in place (with just a few lines of code btw). If some test cases are missing for the code being changed, why not fill this gap for the team and the project to benefit? No one forces us to stick to the "original behavior". No one forces us not to to extend the scope of the PR slightly to improve code quality and test coverage. But what I believe what we as a team force each other to do (and keep each other accountable for doing it) is to stick to the highest code quality standards possible every time, every LoC and every PR.
If I'm being completely honest, I don't see why it might be a problem. https://github.com/eigerco/beerus/pull/630/files#r1574811607 |
BREAKING CHANGE:
Config::from_envnow returns aneyre::Resultinstead of panicking on site.The primary motivation is #630 , but I also wanted more explicit messages when running Beerus without specifying the
STARKNET_RPCandETH_EXECUTION_RPCkeys.