-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/configuration #67
Conversation
berendsliedrecht
commented
Feb 25, 2022
•
edited by karimStekelenburg
Loading
edited by karimStekelenburg
- View and initialise a configuration file (NEEDS WINDOWS SUPPORT)
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIsh I could be more critical🤷♂️ but looks good @blu3beri!
cli/src/error.rs
Outdated
@@ -7,6 +7,7 @@ pub enum Error { | |||
NoEndpointSupplied, | |||
NoConfigKey, | |||
UnqualAmountKeyValue, | |||
ConfigAlreadyExists, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe ConfigExists
is more concise with same expressiveness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah its more terse, I like it :)
cli/src/modules/configuration.rs
Outdated
} | ||
|
||
unreachable!() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand (yet) how this unreachable works here. But that's not say say change it more a "I'm curious"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unreachable macro panics when it is reached.
But i dont actually think we want to panic but exit gracefully. I will change it actually.
pub async fn parse_configuration_args(options: &ConfigurationOptions, _logger: Log) -> Result<()> { | ||
pub async fn parse_configuration_args(options: &ConfigurationOptions, logger: Log) -> Result<()> { | ||
let home = env!("HOME"); | ||
let default_config_path = Path::new(home).join(".config/aries-cli/config.ini"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I like this. Maybe in the future we can also give it a custom path and use this one if none is supplied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think that that is a smart idea. It introduces persistent state of where the default configuration file is located. Something That is hard to manage. We could allow for it, BUT without it being a default. Could you make an issue for this? We can pick it up after a v1 release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I totally agree actually. They way I thought about it.why I mentioned it was that quite a few people have their own "best" way of managing configs and dot file (I know they can always just use a symlink or sth) but probs would make users happy to be able to choose. That said - yeah it makes it more error-prone for gaining little funtionality
Signed-off-by: Berend Sliedrecht <[email protected]>