Skip to content

refactor(protocol): derive image format name parsing - #503

Merged
eval-exec merged 1 commit into
mainfrom
refactor/image-format-name-parsing
Oct 7, 2026
Merged

eval-exec merged 1 commit into
mainfrom
refactor/image-format-name-parsing

Conversation

@eval-exec

Copy link
Copy Markdown
Owner

ImageFormatName::from_lisp_type duplicated the enum’s 12 known image types in a handwritten string parser. Derive strum::EnumString with kebab-case names and #[strum(default)] Other(String) so the enum declaration defines recognition and captures unknown inputs.

Keep the existing infallible from_lisp_type API by forwarding to the generated From<&str> implementation. Preserve exact case-sensitive recognition and unknown-name contents. GNU diagnostic spellings and signature-mismatch policy remain unchanged. Reuse the existing strum dependency.

Validation: all 852 protocol library tests pass. Tests pin all 12 known symbols and diagnostic spellings, plus unknown inputs including case changes, underscores, whitespace, empty strings, and Unicode. Formatting and diff checks pass. Tests remain in image_diagnostic/tests/diagnostic_test.rs.

Copilot AI balanced review requested due to automatic review settings October 7, 2026 17:54
@coderabbitai

coderabbitai Bot commented Oct 7, 2026

Copy link
Copy Markdown

Review in Change Stack →

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 7d78c8d4-e7af-4dbe-afd9-7c5e3c3a1c1e
📥 Commits

Reviewing files that changed from the base of the PR and between c17e6c6 and 8c91e97.

📒 Files selected for processing (2)
  • crates/neomacs-display-protocol/src/image_diagnostic.rs
  • crates/neomacs-display-protocol/src/image_diagnostic/tests/diagnostic_test.rs
 _____________________
< 🧯 Bug extinguished. >
 ---------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@eval-exec
eval-exec merged commit d7de50f into main Oct 7, 2026
27 of 34 checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

1 open finding
What changed in this PR

Refactors ImageFormatName parsing to be derived from the enum definition (via strum::EnumString) and expands tests to pin both recognized formats and “unknown” passthrough behavior.

Changes:

  • Derive strum::EnumString on ImageFormatName with kebab-case serialization and a #[strum(default)] Other(String) fallback.
  • Simplify ImageFormatName::from_lisp_type to delegate to the derived parsing.
  • Expand diagnostic tests to cover all known symbols plus unknown/case/whitespace/Unicode inputs.
File Description
crates/​neomacs-display-protocol/​src/​image_diagnostic.rs Switches handwritten string matching to strum-derived enum parsing with a default Other(String) fallback.
crates/​neomacs-display-protocol/​src/​image_diagnostic/​tests/​diagnostic_test.rs Reworks tests into table-driven cases for all known types and adds coverage for unknown symbol preservation.

🧠 Review effort: Lite


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

"native-image" => Self::NativeImage,
other => Self::Other(other.to_owned()),
}
Self::from(name)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants