Skip to content

Conversation

lmondada
Copy link
Contributor

@lmondada lmondada commented Apr 15, 2025

It is currently impossible to obtain the map from nodes in the replacement graph to newly inserted nodes in self from the ApplyResult type for SimpleReplacement.

This PR fixes this.

BREAKING CHANGE: The ApplyResult associated type of SimpleReplacement is now a custom struct. The deleted nodes returned previously can be obtained using the result.deleted_nodes field.

@lmondada lmondada requested a review from a team as a code owner April 15, 2025 16:40
@lmondada lmondada requested a review from acl-cqc April 15, 2025 16:40
@lmondada lmondada added the breaking-change Changes that break semver label Apr 15, 2025
@lmondada lmondada added this to the hugr-rs 0.16 milestone Apr 15, 2025
@lmondada lmondada requested a review from Copilot April 15, 2025 16:42
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Copy link

codecov bot commented Apr 15, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 82.90%. Comparing base (7626797) to head (4b836c9).
Report is 2 commits behind head on feat/rewrite-trait.

Files with missing lines Patch % Lines
hugr-core/src/ops/custom.rs 50.00% 3 Missing ⚠️
hugr-passes/src/lower.rs 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##           feat/rewrite-trait    #2085      +/-   ##
======================================================
- Coverage               82.91%   82.90%   -0.01%     
======================================================
  Files                     217      217              
  Lines                   41529    41536       +7     
  Branches                37707    37714       +7     
======================================================
+ Hits                    34433    34437       +4     
- Misses                   5292     5295       +3     
  Partials                 1804     1804              
Flag Coverage Δ
python 85.40% <ø> (ø)
rust 82.65% <80.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@acl-cqc acl-cqc left a comment

Choose a reason for hiding this comment

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

Thanks @lmondada - seems a good idea regardless!

This looks fine to me (modulo comments), but it's gonna conflict with #2070. I've approved on the assumption that you'll merge this first (suggested changes are quite trivial)....

@lmondada lmondada changed the base branch from main to feat/rewrite-trait April 21, 2025 16:32
@lmondada lmondada force-pushed the feat/simplereplace-return branch 2 times, most recently from 855f60e to 4b836c9 Compare April 21, 2025 16:36
@lmondada lmondada force-pushed the feat/simplereplace-return branch from 4b836c9 to 706a887 Compare April 21, 2025 16:43
@lmondada lmondada merged commit f664560 into feat/rewrite-trait Apr 21, 2025
@lmondada lmondada deleted the feat/simplereplace-return branch April 21, 2025 16:44
@lmondada
Copy link
Contributor Author

Given that we are delaying the merge of all breaking change PRs, I've chosen to merge this into #2070 and already resolve the merge conflict.

@acl-cqc
Copy link
Contributor

acl-cqc commented Apr 22, 2025

Oh - sorry I didn't spot what you were doing here - I'd have suggested merging into the release-rs-v0.16.0 branch, and then targetting #2070 onto that branch too

lmondada added a commit that referenced this pull request Apr 28, 2025
It is currently impossible to obtain the map from nodes in the
replacement graph to newly inserted nodes in `self` from the
`ApplyResult` type for `SimpleReplacement`.

This PR fixes this.

BREAKING CHANGE: The `ApplyResult` associated type of
`SimpleReplacement` is now a custom struct. The deleted nodes returned
previously can be obtained using the `result.deleted_nodes` field.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change Changes that break semver

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants