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: Update expression checker to use new diagnostics #587

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

mark-koch
Copy link
Collaborator

Closes #539

@mark-koch mark-koch requested a review from a team as a code owner October 22, 2024 16:45
@mark-koch mark-koch requested review from doug-q and removed request for a team October 22, 2024 16:45
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 92.85714% with 15 lines in your changes missing coverage. Please review.

Project coverage is 91.89%. Comparing base (bf60207) to head (c56c7c8).

Files with missing lines Patch % Lines
guppylang/checker/expr_checker.py 92.78% 15 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           diag/jupyter     #587      +/-   ##
================================================
+ Coverage         91.70%   91.89%   +0.18%     
================================================
  Files                61       61              
  Lines              6511     6675     +164     
================================================
+ Hits               5971     6134     +163     
- Misses              540      541       +1     

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

Base automatically changed from diag/jupyter to feat/diagnostics October 29, 2024 09:35
Copy link
Contributor

@doug-q doug-q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

I would prefer putting sub-diagnostics logic inside the Error classes rather than have callers explictly add_sub_diagnostic, but if you prefer it this way that's fine.



@dataclass(frozen=True)
class UnexpectedArgumentError(Error):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems to duplicate the one above and It doesn't seem to be used?

sources.add_file(file, source)
sources.add_file(file, defn.cell_source)
else:
# If we couldn't find the defining cell, just use the source code we
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is a drive by to fix a bug in one of the Jupyter notebook tests: source only holds the source of the function definition, but the line numbers in the AST correspond to the whole cell

@mark-koch
Copy link
Collaborator Author

I would prefer putting sub-diagnostics logic inside the Error classes rather than have callers explictly add_sub_diagnostic, but if you prefer it this way that's fine.

I prefer the current design since some sub diagnostics are only added conditionally (e.g. notes on linearity errors). But I agree, it would be nice to have a pattern for sub diagnostics that are always added. I created issue #620 to implement this

@mark-koch mark-koch merged commit d6ad6b4 into feat/diagnostics Nov 5, 2024
2 checks passed
@mark-koch mark-koch deleted the diag/expr-checker branch November 5, 2024 10:46
github-merge-queue bot pushed a commit that referenced this pull request Nov 11, 2024
This is the development branch for our new diagnostics infrastructure.
Tracked by #535.

*  #548
* #551
* #552 
* #553
* #586
* #588
* #587
* #589 
* #590
* #600
* #601
* #604 
* #605 
* #606
github-merge-queue bot pushed a commit that referenced this pull request Nov 12, 2024
🤖 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]>
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