-
Notifications
You must be signed in to change notification settings - Fork 2
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: Take all spans into account when determining snippet alignment #588
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/diagnostics #588 +/- ##
====================================================
- Coverage 91.73% 91.70% -0.03%
====================================================
Files 61 61
Lines 6509 6513 +4
====================================================
+ Hits 5971 5973 +2
- Misses 538 540 +2 ☔ View full report in Codecov by Sentry. |
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.
Looks good to me - basically, I am just feeling silly that I was confused. But a couple of suggestions if you like :)
guppylang/diagnostic.py
Outdated
@@ -208,10 +208,16 @@ def render_diagnostic(self, diag: Diagnostic) -> None: | |||
else: | |||
span = to_span(diag.span) | |||
level = self.level_str(diag.level) | |||
all_spans = [ | |||
span, | |||
*(to_span(child.span) for child in diag.children if child.span), |
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.
nit: took me awhile to figure out what this *
was about - it does seem like there's one more construction/destruction than I'd expect. It might be less efficient but [span] + [to_span(child.span) for child in diag.children if child.span]
might be clearer. Up to you!
99 | apple == orange | ||
| ^^ Comparison attempted here | ||
| | ||
100 | apple == orange |
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.
This confused me a bit too - I realize this is purely formatting, but this looks like the subdiagnostic should also refer to line 99! (What happens if it does? Is that a separate test worth having?)
Changing the "This is an apple" to "This apple is on another line" might have avoided confusion ;)
🤖 I have created a release *beep* *boop* --- ## [0.13.0](v0.12.2...v0.13.0) (2024-11-12) ### ⚠ BREAKING CHANGES * `prelude` module renamed to `std` ### Features * add `qubit` discard/measure methods ([#580](#580)) ([242fa44](242fa44)) * Add `SizedIter` wrapper type ([#611](#611)) ([2e9da6b](2e9da6b)) * conventional results post processing ([#593](#593)) ([db96224](db96224)) * Improve compiler diagnostics ([#547](#547)) ([90d465d](90d465d)), closes [#551](#551) [#553](#553) [#586](#586) [#588](#588) [#587](#587) [#590](#590) [#600](#600) [#601](#601) [#606](#606) * restrict result tag sizes to 256 bytes ([#596](#596)) ([4e8e00f](4e8e00f)), closes [#595](#595) ### Bug Fixes * Mock guppy decorator during sphinx builds ([#622](#622)) ([1cccc04](1cccc04)) ### Documentation * Add DEVELOPMENT.md ([#584](#584)) ([1d29d39](1d29d39)) * Fix docs build ([#639](#639)) ([bd6011c](bd6011c)) ### Miscellaneous Chores * Manually set last release commit ([#643](#643)) ([b2d569b](b2d569b)) ### Code Refactoring * rename prelude to std ([#642](#642)) ([1a68e8e](1a68e8e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: Agustín Borgna <[email protected]>
No description provided.