Skip to content

Conversation

@CTTY
Copy link
Collaborator

@CTTY CTTY commented May 12, 2025

Which issue does this PR close?

What changes are included in this PR?

  • Added new API set_location in transaction

Are these changes tested?

  • Added an unit test to cover the change

);
}

#[test]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make a integration test to make sure SetLocation works after the commit?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we do not have one yet

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this makes sense, many transaction APIs lack an integ test to verify them. I've created this issue to track the task of adding this integ test: #1322

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thanks @CTTY for this pr, LGTM!

@liurenjie1024 liurenjie1024 merged commit 1373d7f into apache:main May 13, 2025
17 checks passed
@CTTY CTTY deleted the ctty/tx-setloc-new branch May 13, 2025 17:09
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.

Support set_location in transaction

3 participants