[fm] handle multi-line comment fields in FM displayers#10710
Conversation
hawkw
added
the
fault-management
There was a problem hiding this comment.
Pull request overview
This PR updates the fault-management (nexus_types::fm) multi-line fmt::Display output to correctly render comment fields that contain multiple lines by emitting each line with consistent indentation and a leading //.
Changes:
- Updated multiple FM
Displayimplementations to rendercommentvalues line-by-line viacomment.lines(). - Standardized comment rendering in case display sections (ereports/alerts/bundles) to use
//-prefixed lines. - Updated analysis report and log entry displayers to support multi-line comments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| nexus/types/src/fm/case.rs | Switches several case-related display sections to emit multi-line comments with // prefixes and consistent indentation. |
| nexus/types/src/fm/analysis_reports.rs | Updates analysis-report and log-entry display formatting so multi-line comments render as multiple indented // lines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| writeln!(f, "{BULLET:>indent$}ereport {}", ereport.id)?; | ||
| for line in comment.lines() { | ||
| writeln!(f, "{:>indent$}// {line}", "")?; | ||
| } |
| writeln!(f, "{BULLET:>indent$}alert {id}",)?; | ||
| for line in comment.lines() { | ||
| writeln!(f, "{:>indent$}// {line}", "")?; | ||
| } |
There was a problem hiding this comment.
thanks i changed this on purpose
| writeln!(f, "{BULLET:>indent$}bundle {id}",)?; | ||
|
|
||
| for line in comment.lines() { | ||
| writeln!(f, "{:>indent$}// {line}", "")?; | ||
| } |
| writeln!(f, "{BULLET:>indent$}ereport {}", ereport.id)?; | ||
| for line in comment.lines() { | ||
| writeln!(f, "{:>indent$}// {line}", "")?; | ||
| } |
| let colon = if kvs.is_empty() { "" } else { ":" }; | ||
| writeln!(f, "{:indent$}{bullet}{event}{colon}", "")?; | ||
| if let Some(comment) = comment { | ||
| writeln!(f, "{:indent$} // {comment}", "")?; | ||
| for line in comment.lines() { | ||
| writeln!(f, "{:indent$} // {line}", "")?; | ||
| } |
| @@ -84,8 +84,8 @@ impl AnalysisReport { | |||
| indent, | |||
There was a problem hiding this comment.
Thanks for pulling this into a new PR; appreciate it being decoupled
| this_sitrep(*assigned_sitrep_id) | ||
| )?; | ||
| writeln!(f, "{:>indent$}{ASSIGNMENT_ID:<WIDTH$} {id}", "")?; | ||
| writeln!(f, "{:>indent$}{COMMENT:<WIDTH$} {comment}\n", "",)?; |
There was a problem hiding this comment.
... I think copilot was also saying something to this effect, but do we want to add a writeln!(f, "")? here?
We took out the \n that coincidentally used to be after the comment, so now all the CaseEreports will be smushed together
There was a problem hiding this comment.
they have bullet points, so i removed the newline since i felt like it was wasting vertical space. i can put it back if you feel strongly about it?
There was a problem hiding this comment.
ah, nah, I didn't realize there was "some separator". Don't care that much
There was a problem hiding this comment.
I think the const str COMMENT is only used here, but we aren't actually printing out the comment: string anywhere anymore. We could probably remove this?
| )?; | ||
| writeln!(f, "{:>indent$}{DATA}", "")?; | ||
| writeln!(f, "{}", data_selection.display(indent + 2))?; | ||
| writeln!(f, "{:>indent$}{COMMENT:<WIDTH$} {comment}\n", "")?; |
There was a problem hiding this comment.
I think copilot is weird with where it's choosing to insert commentary , but the \n that was removed was here. I think I agree; it's worth putting it back?
There was a problem hiding this comment.
ok, i will return the newlines
This branch changes the various multi-line
fmt::Displayimplementations for fault management types innexus_types::fmto handlecommentfields that span multiple lines. I'd like to be able to put multi-line text incommentfields in #10593, and the presentDisplayimpls will wreck it, because subsequent lines won't be indented and will lack the leading//to show that it's part of a comment. This branch fixes that.I originally made this change in #10593, but figured it was worth pulling out into its own little PR so that it could merge separately. I might make some additional changes to the display stuff, and didn't want to have giant merge conflicts later when #10593 merges.