Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Ensure transactional with ? works in frame v2 #7982

Merged
merged 2 commits into from
Jan 26, 2021
Merged

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Jan 26, 2021

actually I'm not sure it is needed to test for it as transactional macro is independant from frame v2, but anyway having it in the test might be better to show usage

cc @apopiak

EDIT: this comes from my missunderstood of Apopiak message, sorry

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jan 26, 2021
@gui1117 gui1117 added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jan 26, 2021
@gui1117 gui1117 merged commit d0723f1 into master Jan 26, 2021
@gui1117 gui1117 deleted the gui-transactional-test branch January 26, 2021 15:42
@xlc
Copy link
Contributor

xlc commented Jan 26, 2021

The underlying issue is fixed with #7301

@gui1117
Copy link
Contributor Author

gui1117 commented Jan 26, 2021

Yes but considering set_value never fails, I think the tests weren't actually triggering the early return ?

@xlc
Copy link
Contributor

xlc commented Jan 26, 2021

The original issue was that the code won't compile if ? is used and that's what the test in #7301 about.

@gui1117
Copy link
Contributor Author

gui1117 commented Jan 26, 2021

ah indeed I see why it wouldn't compile, this PR wasn't needed, sorry

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants