Skip to content
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

migrate remaining errors to new model #108

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

migrate remaining errors to new model #108

wants to merge 7 commits into from

Conversation

asmello
Copy link
Collaborator

@asmello asmello commented Feb 18, 2025

The EncodingError and ParseIndexError types are parsing errors with a
well defined 'subject', but they weren't previously made into Diagnostic
implementors, so they worked a bit inconsistently with other error types. This
PR makes it so those errors follow the same conventions and general structure
used by its kindred.

There is one related breaking change: InvalidCharacterError previously held
a copy of the subject, but in the new model this is a responsibility of its
enriched Report<InvalidCharacterError> type. Since some methods directly
depended on access to the subject, they had to be removed. I think this is
justifiable since the new error APIs are already breaking (we just should've
made this change in tandem with the other related breaks).

I don't have any other error-related breaks planned.

This PR also incidentally fixes an incorrect error type returned if the token ended with a ~ (it would be flagged as a slash error, not tilde). I updated tests to cover this.

@asmello
Copy link
Collaborator Author

asmello commented Feb 18, 2025

TODO: update changelog

@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2025

Codecov Report

Attention: Patch coverage is 92.41706% with 16 lines in your changes missing coverage. Please review.

Project coverage is 95.8%. Comparing base (f3c23b9) to head (df39beb).

Files with missing lines Patch % Lines
src/index.rs 88.0% 13 Missing ⚠️
src/token.rs 96.9% 3 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/assign.rs 97.6% <100.0%> (-0.1%) ⬇️
src/diagnostic.rs 77.9% <ø> (+3.4%) ⬆️
src/token.rs 98.7% <96.9%> (+1.0%) ⬆️
src/index.rs 91.2% <88.0%> (-1.0%) ⬇️

@chanced
Copy link
Owner

chanced commented Feb 25, 2025

Damnit, I really regret pushing 0.7. Agreed that it should have been bundled.

We can cut a 0.8 in short order i think. I doubt many will have picked up 0.7 already and the breaks slotted for it won't be that bad.

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.

3 participants