feat: added upstream miette support#1038
Conversation
WalkthroughThis pull request introduces changes to the Rust project's Changes
Poem
Tip Announcements
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (2)
Additional comments not posted (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- pest/Cargo.toml (2 hunks)
- pest/examples/parens.rs (1 hunks)
- pest/src/error.rs (4 hunks)
Additional comments not posted (6)
pest/Cargo.toml (2)
30-30: Verify the impact of removingmemchrfrom default features.Removing
memchrcould affect performance or functionality. Please ensure that this change is tested thoroughly, especially in parts of the codebase where fast string searching was critical.
30-34: Approve the addition ofmietteas an optional dependency.The addition of
mietteis intended to enhance error reporting capabilities. Ensure thatmietteintegrates well with existing error handling mechanisms and does not introduce any conflicts or issues.pest/examples/parens.rs (1)
71-84: Approve enhanced error handling with conditionalmietteintegration.The changes to error handling in the
mainfunction improve flexibility and clarity. The use of conditional compilation ensures that these enhancements are only active when the "miette" feature is enabled. Consider adding comments explaining the use ofmap_errfor future maintainers.pest/src/error.rs (3)
603-603: Approve the increased visibility of theformatmethod.Changing the visibility of the
formatmethod topuballows for broader usage, potentially with external libraries or other parts of the project. Ensure that the documentation reflects the intended use cases and any potential risks associated with broader access.
675-679: Approve the addition of theinto_miettemethod with conditional compilation.The
into_miettemethod enhances error reporting by convertingErrorinstances intomiette::Diagnosticobjects. This integration is well-aligned with the optionalmiettedependency specified in theCargo.toml. Consider adding unit tests to ensure that this method functions as expected under various conditions.
737-773: Approve the addition of themiette_adaptermodule and its contents.The
MietteAdapterstruct within themiette_adaptermodule effectively integrates theErrortype with themiettelibrary by implementing theDiagnostictrait. This implementation enhances error reporting by providing detailed diagnostic information. Ensure that the implementation covers all necessary aspects of theDiagnostictrait and consider adding more comprehensive tests to cover various error scenarios.
tomtau
left a comment
There was a problem hiding this comment.
thanks!
cargo fmt should fix the small formatting issue: https://github.com/pest-parser/pest/actions/runs/10776713820/job/29909630226?pr=1038
|
Fixed the issues. |
There was a problem hiding this comment.
the miette feature fails on some combination of feature flags: https://github.com/pest-parser/pest/actions/runs/10794522394/job/29941922867?pr=1038#step:4:141 (e.g. cargo check --lib --tests --no-default-features --features miette)
if miette needs std, you can perhaps define an explicit feature flag:
[features]
#...
miette-error = ["std", "dep:miette"]
and change those feature guards accordingly (#[cfg(feature = "miette-error")])
|
The dependencies for miette are fixed. I added |
|
wow what a luck, I just came back to look at the miette integration after months, and 3 days ago this was merged <3 <3 great work! |
Reopening #582
Added tests as mentioned in #663 (comment)
Summary by CodeRabbit
New Features
miettedependency.miette::Diagnosticfor improved diagnostics.Bug Fixes
miettefeature, allowing for clearer error messages.Documentation
miettelibrary.