-
Notifications
You must be signed in to change notification settings - Fork 19
Add config
command
#294
base: main
Are you sure you want to change the base?
Add config
command
#294
Conversation
- wrapped config options with 'Option' type - show config is working fine! - added fn in utils - prettify the config details
…t show debug info
- removed redundant check for MIN_FARM_SIZE - added config command in arrow_key_mode - added usages except node, farm directory
- removed small fn from utils & used the code directly into placeholders - corrected the order of commands in interactive mode
- thorough manual testing of `create_or_move_data` fn was done - added usage for `node-dir` & `farm-dir` params & all params at a time.
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.
Please update against latest main
src/commands/config.rs
Outdated
|
||
// update (optional) the chain | ||
if let Some(c) = chain { | ||
config.chain = ChainConfig::from_str(&c)?; |
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 check if the chain has changed before wiping it?
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.
Although redundant.
Added here in this commit.
Additionally, added check for parsing already set chain.
…nfig-cmd # Conflicts: # Cargo.toml # pulsar/src/commands/config.rs
`$ pulsar config -s` has been replaced with `$ pulsar config` to show the already set config params/fields details.
@@ -175,3 +177,48 @@ fn custom_log_dir_test() { | |||
#[cfg(target_os = "windows")] | |||
assert!(log_path.ends_with("AppData/Local/pulsar/logs")); | |||
} | |||
|
|||
#[cfg(test)] | |||
mod create_or_move_data_tests { |
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.
You can just have mod test
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.
Actually, the aim is to keep the related tests under one mod label.
Let me know your thoughts.
|
||
/// Ensuring the function correctly handles the case where the old and new | ||
/// directories are the same. | ||
#[test] |
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 would prefer to test a scenario where we have actual files in old directory which we move into new and delete the old.
for entry in entries { | ||
let entry = entry?; | ||
let file_name = entry.file_name(); | ||
fs::rename(entry.path(), new_dir.join(file_name))?; |
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.
What would happen if this fails? We are left with some data in old directory and some data in new one. Which means after this happen, user won't be able to use pulsar and have to fix the situation by hand.
I would like to have atomic move implemented here. Either we move everything or we move nothing.
Description
This PR adds
config
command to the CLI tool that would allow user to change (if they wish for) the set params (duringinit
command) like:This also covers the issue: #248 (comment)