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

Add patches support to transactions #331

Closed
wants to merge 4 commits into from
Closed

Add patches support to transactions #331

wants to merge 4 commits into from

Conversation

jeromegn
Copy link
Contributor

@jeromegn jeromegn commented Apr 7, 2022

This adds previously-unsupported patches tracking to transactions. Patches only worked for apply_changes in the past.

@jeromegn jeromegn mentioned this pull request Apr 7, 2022
@jeffa5
Copy link
Collaborator

jeffa5 commented Apr 9, 2022

Thanks for this! Would you be able to add some tests to check that this works and continues to work?

I also think we should be able to get around having to query the opset again for each patch by storing the Prop of each operation in the TransactionInner::operations field and then using that to make the patches.

@jeromegn
Copy link
Contributor Author

I added a test(s)!

I also think we should be able to get around having to query the opset again for each patch by storing the Prop of each operation in the TransactionInner::operations field and then using that to make the patches.

I've also added this (I think I did it right). I'd like to trigger the test pipeline here to make sure it's all good.

@alexjg
Copy link
Collaborator

alexjg commented Apr 10, 2022

I'd like to trigger the test pipeline here to make sure it's all good.

Note that you can run all the tests that CI runs locally by running ./scripts/ci/run

@orionz
Copy link
Collaborator

orionz commented Apr 19, 2022

This is superseded by Op Observer

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.

None yet

4 participants