One-liner #36
One-liner #36
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces one-liner formatting and parsing capabilities for Cirru expressions, primarily intended for CLI usage. The implementation allows converting a single Cirru expression to and from a compact single-line representation without newlines or indentation.
Key changes:
- Added
format_expr_one_linerandparse_expr_one_linerfunctions for one-line expression handling - Added methods directly on the
Cirrustruct for convenient access to one-liner functionality - Version bumped from 0.1.35 to 0.1.37
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/oneliner_test.rs | New test file covering basic one-liner formatting/parsing, multi-statement error handling, and complex nesting scenarios |
| src/writer.rs | Implements generate_statement_one_liner for rendering expressions without top-level parentheses, adds format_expr_one_liner function and redundant CirruOneLinerExt trait |
| src/primes.rs | Adds format_one_liner method to the Cirru struct for convenient one-liner formatting |
| src/parser.rs | Implements parse_expr_one_liner function to parse single expressions, adds CirruOneLinerParseExt trait for ergonomic method-style parsing, exports new public API |
| Cargo.toml | Version bump to 0.1.37 |
| Cargo.lock | Corresponding lock file update for version 0.1.37 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /// Formats this expression into a single line of Cirru code. | ||
| /// | ||
| /// Note: only list expressions can be formatted as an expression. |
There was a problem hiding this comment.
The documentation states "only list expressions can be formatted as an expression", but this phrasing is slightly redundant (using "expression" twice in different senses). Consider rewording to: "Only Cirru::List values can be formatted with this method. For Cirru::Leaf, this returns an error."
| /// Note: only list expressions can be formatted as an expression. | |
| /// Only `Cirru::List` values can be formatted with this method. |
|
|
||
| /// Extension trait for method-style one-liner formatting. | ||
| pub trait CirruOneLinerExt { | ||
| fn format_one_liner(&self) -> Result<String, String>; | ||
| } | ||
|
|
||
| impl CirruOneLinerExt for Cirru { | ||
| fn format_one_liner(&self) -> Result<String, String> { | ||
| format_expr_one_liner(self) | ||
| } | ||
| } |
There was a problem hiding this comment.
The CirruOneLinerExt trait is redundant. The Cirru struct already has a format_one_liner method defined in src/primes.rs (line 125), which delegates to the same format_expr_one_liner function. This trait adds no additional value and creates unnecessary duplication in the API surface. Consider removing this trait and only keeping the method on Cirru itself.
| /// Extension trait for method-style one-liner formatting. | |
| pub trait CirruOneLinerExt { | |
| fn format_one_liner(&self) -> Result<String, String>; | |
| } | |
| impl CirruOneLinerExt for Cirru { | |
| fn format_one_liner(&self) -> Result<String, String> { | |
| format_expr_one_liner(self) | |
| } | |
| } |
| pub use primes::{Cirru, CirruLexItem, CirruLexItemList, escape_cirru_leaf}; | ||
| pub use s_expr::format_to_lisp; | ||
| pub use writer::{CirruWriterOptions, format}; | ||
| pub use writer::{CirruOneLinerExt, CirruWriterOptions, format, format_expr_one_liner}; |
There was a problem hiding this comment.
The CirruOneLinerExt trait is being exported but it's redundant since Cirru already has a format_one_liner method. This export should be removed from the public API to avoid confusion.
| pub use writer::{CirruOneLinerExt, CirruWriterOptions, format, format_expr_one_liner}; | |
| pub use writer::{CirruWriterOptions, format, format_expr_one_liner}; |
| assert_eq!(parsed, tree); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Test coverage should be added for the case where format_expr_one_liner is called with a Cirru::Leaf instead of a Cirru::List. According to the documentation and implementation, this should return an error, but this behavior is not currently tested.
| assert_eq!(parsed, tree); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Test coverage should be added for edge cases such as empty lists and lists with only one element. The current tests only cover well-formed expressions with multiple elements, but edge cases like Cirru::List(vec![]) and Cirru::List(vec![Cirru::Leaf("single".into())]) should also be tested to ensure the one-liner formatter handles them correctly.
Major changes: - Removed warning system (CirruWarning types) - Simplified API: parse() returns Result<Vec<Cirru>, CirruError> - Unicode escape sequences (\u) now return clear errors instead of warnings - Enhanced error messages with escaped special characters (\n, \t, etc.) - Added position tracking with line/column/offset information - Added code snippet extraction with visual pointer - All tests passing (22 tests) - Updated documentation and examples Breaking changes: - parse() signature changed from Result<ParseResult<T>, Error> to Result<T, Error> - Removed ParseResult, CirruWarning, CirruWarningKind types - parse_simple() deprecated (use parse() directly) - Unicode escapes now fail instead of generating warnings
...mostly for CLI usages