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(api): Allow adding/removing tips on HW API without await #16530

Merged
merged 6 commits into from
Oct 18, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Oct 18, 2024

Changelog

  • Turn API.remove_tip() and OT3API.remove_tip() non-async. This will help with EXEC-779 by letting us keep the whole auto-fixup process non-async.
  • Same for .add_tip(). This isn't directly helpful to us because we use .cache_tip() anyway, but it's nice for consistency.
  • A couple other minor cleanups.

Test Plan and Hands on Testing

  • Make sure all CI keeps passing.

Review requests

  • Is there any reason we would want these methods to be async?
  • Is it sufficient to ensure that all of CI keeps passing, or are there hidden things that this might break?

Risk assessment

Low if CI covers everything that calls this.

@SyntaxColoring SyntaxColoring requested a review from a team October 18, 2024 15:02
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner October 18, 2024 15:02
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Looks good. I expect ci is fine. I think these were made async in the past for looking-forward-to-IPC-implementation reasons.

@SyntaxColoring SyntaxColoring merged commit 7b023dc into edge Oct 18, 2024
28 checks passed
@SyntaxColoring SyntaxColoring deleted the non_async_tip_state branch October 18, 2024 15:27
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.

2 participants