Skip to content

make PointerBuf::parse error basic by default #103

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

Closed
wants to merge 7 commits into from

Conversation

asmello
Copy link
Collaborator

@asmello asmello commented Feb 16, 2025

Since #101 the assign and resolve Error types are self-contained and
shallow by default (meaning they don't include a copy of their source string).
This is nice - the errors are helpful and don't require extra allocations, but
the users may still opt into richer errors by using the Diagnose trait and
providing the "source code" (in miette terminology).

The PointerBuf::parse error was different, however. It was made a rich error
by default, which requires one extra clone (which may be unnecessary), but
more importantly, is inconsistent with the other error contract.

This PR makes the parse error consistent, by switching it back to ParseError.
To recover the rich error functionality users can just call .diagnose() on the
Result, same as with assign::Error and resolve::Error.

I'm also sneaking in a fix for the eager behaviour of Diagnose::diagnose_with.

@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2025

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.8%. Comparing base (c0201c7) to head (d191e24).

Files with missing lines Patch % Lines
src/diagnostic.rs 0.0% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/pointer.rs 95.6% <100.0%> (-0.1%) ⬇️
src/diagnostic.rs 74.4% <0.0%> (ø)

@asmello
Copy link
Collaborator Author

asmello commented Feb 16, 2025

Aha, I think I figured out (rediscovered?) why PointerBuf::parse was special cased. It means when using the rich error interface, then allocation always take place. This is not too unreasonable, but I'll put some thought into whether it can be avoided.

@asmello
Copy link
Collaborator Author

asmello commented Feb 17, 2025

A couple options:

  1. Make the error an enum:
enum ParseErrorWrapper {
    Simple(ParseError),
    Rich(Report<ParseError>)
}

and pivot based on the Cow variant. With this it makes sense to actually have a Cow rather than use the CowStr trait.
2. Optionally keep the String inside ParseError, then make it so into_report can use the internal subject rather than a provided one. This would require a trait refactor, not sure how it'd look exactly. It also means making ParseError a struct so we can hide the String field (not strictly necessary, but I think it's a good idea).

Either way there's some amount of user-facing complexity added, but on the other end this is kinda dumb:

    let ptr_str = "hello";
    PointerBuf::parse(ptr_str).diagnose_with(|| ptr_str.to_owned()); // kinda fine
    let ptr_str = String::from(ptr_str);
    PointerBuf::parse(&ptr_str).diagnose(ptr_str); // dumb, we allocate in Ok case unecessarily

Ugh, I may reconsider this PR entirely.

@asmello
Copy link
Collaborator Author

asmello commented Feb 17, 2025

Ok, let's drop this. I'll make a different PR with the side-benefits of this one.

@asmello asmello closed this Feb 17, 2025
@asmello
Copy link
Collaborator Author

asmello commented Feb 17, 2025

Side-benefits over at #104

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