-
-
Notifications
You must be signed in to change notification settings - Fork 262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 removingmemchr
from default features.Removing
memchr
could 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 ofmiette
as an optional dependency.The addition of
miette
is intended to enhance error reporting capabilities. Ensure thatmiette
integrates 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 conditionalmiette
integration.The changes to error handling in the
main
function 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_err
for future maintainers.pest/src/error.rs (3)
603-603
: Approve the increased visibility of theformat
method.Changing the visibility of the
format
method topub
allows 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_miette
method with conditional compilation.The
into_miette
method enhances error reporting by convertingError
instances intomiette::Diagnostic
objects. This integration is well-aligned with the optionalmiette
dependency 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_adapter
module and its contents.The
MietteAdapter
struct within themiette_adapter
module effectively integrates theError
type with themiette
library by implementing theDiagnostic
trait. This implementation enhances error reporting by providing detailed diagnostic information. Ensure that the implementation covers all necessary aspects of theDiagnostic
trait and consider adding more comprehensive tests to cover various error scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
miette
dependency.miette::Diagnostic
for improved diagnostics.Bug Fixes
miette
feature, allowing for clearer error messages.Documentation
miette
library.