feat(merod): revamp config command with printing, hints, and safe editing #1274
Conversation
crates/merod/src/cli/config.rs
Outdated
| fn print_diff(&self, old: &str, new: &str) -> EyreResult<()> { | ||
| let diff = TextDiff::from_lines(old, new); | ||
| let mut stdout = StandardStream::stdout(ColorChoice::Auto); | ||
|
|
||
| // Save the updated TOML back to the file | ||
| write(&path, doc.to_string()).await?; | ||
| for op in diff.ops() { | ||
| for change in diff.iter_changes(op) { | ||
| let (sign, color) = match change.tag() { | ||
| ChangeTag::Delete => ("-", TermColor::Red), | ||
| ChangeTag::Insert => ("+", TermColor::Green), | ||
| ChangeTag::Equal => (" ", TermColor::White), | ||
| }; | ||
|
|
||
| info!("Node configuration has been updated"); | ||
| stdout.set_color(ColorSpec::new().set_fg(Some(color)).set_intense(true))?; | ||
| write!(&mut stdout, "{}{}", sign, change)?; | ||
| } |
There was a problem hiding this comment.
see the udiff module
There was a problem hiding this comment.
why was this marked as resolved?
crates/merod/src/cli/config.rs
Outdated
| Item::ArrayOfTables(_) => { | ||
| // Handle arrays if needed | ||
| } |
There was a problem hiding this comment.
yes, they are.
you can use this.is.an.[0].item to represent accessing them
sorta like jq, jsonpath
There was a problem hiding this comment.
this is not resolved, there's no way to edit items inside arrays
crates/merod/src/cli/config.rs
Outdated
| if old_value.to_string() != current[last_key].to_string() { | ||
| changes_made = true; | ||
| } |
There was a problem hiding this comment.
what happens if I specify any of these three:
a.b.c=3wherea.bdid not previously exist?
I suspect panic?
2.a.b.c=3 where a.b.c was previously an object
it'll probably be overwritten, but then validated at a later step to be invalid, which is tolerable
a.b.c=3 a.b.c.d=5
a.b.c will probably discard 3 that was set, and make a.b.c an object with .d inside of it
Can you confirm these?
There was a problem hiding this comment.
this was a prompt, it should be answered, the answers will then inform any further feedback
| Default, | ||
| Toml, | ||
| Json, | ||
| Human, |
There was a problem hiding this comment.
we don't need an explicit human format, since not all cases have one
when viewing, Default is interpreted to mean "toml"
when editing, Default is interpreted to mean "show diff"
when hinting, Default is interpreted to mean "human readable"
|
@miraclx I attempted a different method. Kindly take a look! |
crates/merod/src/cli/config.rs
Outdated
| fn print_diff(&self, old: &str, new: &str) -> EyreResult<()> { | ||
| let diff = TextDiff::from_lines(old, new); | ||
| let mut stdout = StandardStream::stdout(ColorChoice::Auto); | ||
|
|
||
| // Save the updated TOML back to the file | ||
| write(&path, doc.to_string()).await?; | ||
| for op in diff.ops() { | ||
| for change in diff.iter_changes(op) { | ||
| let (sign, color) = match change.tag() { | ||
| ChangeTag::Delete => ("-", TermColor::Red), | ||
| ChangeTag::Insert => ("+", TermColor::Green), | ||
| ChangeTag::Equal => (" ", TermColor::White), | ||
| }; | ||
|
|
||
| info!("Node configuration has been updated"); | ||
| stdout.set_color(ColorSpec::new().set_fg(Some(color)).set_intense(true))?; | ||
| write!(&mut stdout, "{}{}", sign, change)?; | ||
| } |
There was a problem hiding this comment.
why was this marked as resolved? 6D4E
| for change in diff.iter_all_changes() { | ||
| match change.tag() { | ||
| ChangeTag::Delete => print!("-{}", change.red()), | ||
| ChangeTag::Insert => print!("+{}", change.green()), | ||
| ChangeTag::Equal => print!(" {}", change), | ||
| } | ||
| } |
There was a problem hiding this comment.
again, use the udiff module
the format you're printing here is not a diff-style output, so it's not compatible with delta and tools like it
| .into_iter() | ||
| .partition(|kv| kv.key.ends_with('?')); | ||
| let original_doc = toml_str.parse::<toml_edit::DocumentMut>()?; | ||
| let mut modified_doc = original_doc.clone(); |
There was a problem hiding this comment.
looks like we can move this to L101?
crates/merod/src/cli/config.rs
Outdated
| let tmp_dir = temp_dir(); | ||
| let tmp_path = tmp_dir.join(CONFIG_FILE); | ||
| write(&tmp_path, doc.to_string()).await?; | ||
|
|
||
| if first != self.path { | ||
| return None; | ||
| } | ||
| let tmp_path_utf8 = Utf8PathBuf::try_from(tmp_dir)?; | ||
| drop(ConfigFile::load(&tmp_path_utf8).await?); |
There was a problem hiding this comment.
a temp file is not necessary here, check the implementation of ConfigFile::load, it's pretty much just a deserialization - you can do the same here.
if you can also verify toml.parse::<DocumentMut> is equivalent to toml::from_str, that'll be great
crates/merod/src/cli/config.rs
Outdated
| if old_value.to_string() != current[last_key].to_string() { | ||
| changes_made = true; | ||
| } |
There was a problem hiding this comment.
this was a prompt, it should be answered, the answers will then inform any further feedback
| write!(&mut output, "# {}\n", key)?; | ||
| for (k, v) in table.iter() { | ||
| write!(&mut output, "# .{}: {}\n", k, self.type_hint(v))?; | ||
| } |
There was a problem hiding this comment.
the toml output here will be fully commented out?
| PrintFormat::Json => println!( | ||
| "{}", | ||
| serde_json::to_string_pretty(&json!({ |
There was a problem hiding this comment.
| Item::Value(value) => { | ||
| println!("{} = {}", section.bold(), self.format_value(value)); | ||
| } | ||
| _ => {} |
There was a problem hiding this comment.
None? let's be explicit, so we know for a fact we're not missing anything
| fn print_table(&self, table: &toml_edit::Table, indent: usize) -> EyreResult<()> { | ||
| let indent_str = " ".repeat(indent * 2); |
There was a problem hiding this comment.
as mentioned here, when viewing "default == toml"
but I'm curious to see what this looks like (attach some pictures please, if you don't mind)
crates/merod/src/cli/config.rs
Outdated
| Item::ArrayOfTables(_) => { | ||
| // Handle arrays if needed | ||
| } |
There was a problem hiding this comment.
this is not resolved, there's no way to edit items inside arrays
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
bump |
Description
Attempts to resolve #1069
This PR overhauls the merod config command to:
Safe Editing by Default
No longer modifies config files unless --save is explicitly specified
Shows diffs of proposed changes before saving
Enhanced Printing
Adds --print flag with output formats: toml (default), json, human
Prints full config when no edits are requested
Schema Hints
Supports ? syntax to show config structure and documentation
Displays type information and descriptions for nested keys
Added structured output showing types and descriptions
Improved UX
Clear warnings when changes aren't persisted
Colorized diff output
Validation before saving
Test plan
Documentation update