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

fix: add spans for import errors #80

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

tardyp
Copy link
Contributor

@tardyp tardyp commented Aug 12, 2024

Fixes #2

This is my initial attempt at fixing #2.

image

There are a few design issues that I'd like to discuss:

  • the ImportError is arguably part of the api for FileResolver. I choose to rename the original to FileError. Another option is to make the Source and span optional, and have an api like with_source_and_span()
  • The find_span function should be an utility function elsewhere. I am not sure where to put it.

Not in scope of #2

  • ideally we should report all import error for a file, here we only report the first.

@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2024

Codecov Report

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

Project coverage is 92.62%. Comparing base (5446318) to head (c596c6e).
Report is 1 commits behind head on main.

Files Patch % Lines
protox/src/error.rs 96.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #80      +/-   ##
==========================================
- Coverage   92.65%   92.62%   -0.04%     
==========================================
  Files          19       19              
  Lines        2384     2415      +31     
==========================================
+ Hits         2209     2237      +28     
- Misses        175      178       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewhickman andrewhickman merged commit 8775a9a into andrewhickman:main Aug 15, 2024
5 checks passed
@andrewhickman
Copy link
Owner

Awesome, thank you!

@tardyp
Copy link
Contributor Author

tardyp commented Aug 20, 2024

Sorry about the fmt and clippy issues. I wanted to post early to get feedback about the design.

Apperently the design was good enough :)

I'd appreciate a release with this fix, so that my users can take advantage of those better error messages.
They already do like a lot the improvement over protoc.

@andrewhickman
Copy link
Owner

I released version 0.7.1 with this change

@tardyp tardyp deleted the import_spans branch August 20, 2024 16:09
@tardyp
Copy link
Contributor Author

tardyp commented Aug 20, 2024

ah! I checked the tags but not the crate.io...

you forgot to push a tag.

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.

Nicer error messages in build scripts
3 participants