feat: add registry-driven extension parsing and formatting#40
feat: add registry-driven extension parsing and formatting#40wackywendell wants to merge 10 commits intomainfrom
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wackywendell
force-pushed
the
extensions
branch
2 times, most recently
from
1f17b29 to
fe7a50d
Compare
wackywendell
force-pushed
the
extensions
branch
from
041da34 to
2b393dd
Compare
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
Adds support for custom extension relations in the parser and textify system. Includes extension registry, Any encoding/decoding, and conversion utilities.
Enhances linting, adds test coverage, and completes extension relation refactoring with improved textify support.
Centralizes relation parsing context, cleans clippy warnings, and simplifies the Parser API to enforce hard errors for extension parsing.
Moves name() into Explainable trait, simplifies Any/AnyConvertible, cleans up extension errors, and improves examples with ErrorList usage.
Simplifies trait bounds and Any implementation, removes debug prints, simplifies main.rs, enables cli feature by default, and cleans up extension naming.
Introduces ArgsExtractor for easier argument validation and simplifies the extension args API with expect_named_arg and get_named_or helpers.
wackywendell
force-pushed
the
extensions
branch
from
2b393dd to
289763a
Compare
Add comprehensive documentation for the custom extension type system: - Document the three requirements: prost::Message, prost::Name, Explainable - Document the Explainable trait and ArgsExtractor helpers - Add a compilable code example using the ExtensionRegistry - Reference examples/extensions.rs for complete implementation - Update GRAMMAR.md with Extension Relations section
|
@codex review |
|
@cursor review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 062efe0d0b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Rule::expression => { | ||
| // TODO: Handle expressions properly when ExtensionColumn::Expression is implemented | ||
| unimplemented!("Expression handling in ExtensionColumn not yet implemented") | ||
| } |
There was a problem hiding this comment.
The grammar allows expression in extension_column (see src/parser/expression_grammar.pest), but this branch calls unimplemented!, so a valid plan containing an expression output column will panic the parser instead of returning a ParseError. This is a runtime crash path for user-supplied input; the branch should return a structured parse error (or support an expression column variant) rather than aborting.
Useful? React with 👍 / 👎.
| Rule::expression => { | ||
| // TODO: Handle expressions properly when ExtensionValue::Expression is implemented | ||
| // For now, convert to string representation | ||
| ExtensionValue::String(inner.as_str().to_string()) |
There was a problem hiding this comment.
extension_argument accepts expression, but this code converts every expression into ExtensionValue::String, which silently changes semantics for inputs like expr=add($0,1). That loses argument type information and causes round-tripped output to become a quoted string literal rather than an expression, so registry handlers cannot reliably distinguish expressions from actual strings. Unsupported expression arguments should fail parsing until an expression variant is implemented.
Useful? React with 👍 / 👎.
|
|
||
| fn textify<S: Scope, W: fmt::Write>(&self, _ctx: &S, w: &mut W) -> fmt::Result { | ||
| match self { | ||
| ExtensionValue::String(s) => write!(w, "'{s}'"), |
There was a problem hiding this comment.
Extension string values are emitted as '{s}' without escaping, so values containing quotes, backslashes, or control characters produce invalid text or reparsed values that differ from the original payload. This breaks round-trip reliability for extension relations whenever string arguments contain special characters. Use the existing escaping path used elsewhere in textification before writing extension string literals.
Useful? React with 👍 / 👎.
2 participants
Description
ExtensionArgs,ExtensionValue,Any) so parser and formatter can translate extension relations to typed protobuf payloads.ParseError::ExtensionDetailfor registry failures.Type of Change
Testing
Checklist
Related Issues
Closes #32