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

refactor a bit #100

Draft
wants to merge 1 commit into
base: am/new-errors
Choose a base branch
from
Draft

refactor a bit #100

wants to merge 1 commit into from

Conversation

chanced
Copy link
Owner

@chanced chanced commented Nov 28, 2024

Hey @asmello, I made some changes for you to review. High level:

  • renames SRC generic of Report to D and bound it to Diagnostic
  • drops SUB in generic, uses D::Subject instead
  • removes lifetime from Report
  • removes seal for Diagnostic
  • Adds associated type Related and method related to Diagnostic for multi-errors

Regarding lifetimes, I really think we should only deal with owned subjects. It simplifies the code considerably, reduces boilerplate, avoids borrowck issues, and doesn't cost much; a single allocation at worst on an error path for reporting seems more than fair. Given that this behavior is opt-in, I say we keep it simple.

I really don't know about the multi-error list. We can include it and I made some changes to support it, but I don't know how valuable it'll be. However, it is possible with this structure. To support it, we will need a struct ParseErrors(Box<[ParseError]> that would have a Related = ParseError and I'll need to move over the logic from the other branch.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 60.86957% with 9 lines in your changes missing coverage. Please review.

Project coverage is 96.0%. Comparing base (1404c61) to head (d844954).

Files with missing lines Patch % Lines
src/diagnostic.rs 57.1% 9 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/assign.rs 99.0% <ø> (ø)
src/pointer.rs 96.3% <100.0%> (-0.4%) ⬇️
src/diagnostic.rs 74.7% <57.1%> (-3.1%) ⬇️

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