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): Emit proper comment commands from legacy protocols #15419

Closed
wants to merge 2 commits into from

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Jun 14, 2024

Overview

For historical reasons, Python Protocol API ProtocolContext.comment("...")s have always been reported as "legacy" custom commands in the analysis engine and HTTP API. #15408 changes them to proper Protocol Engine comment commands when the Python protocol has apiLevel ≥2.14. This PR makes the same change for Python protocols with apiLevel ≤2.13, just for the sake of uniformity.

Test Plan

I'll test this in #15409.

Review requests

None in particular.

Risk assessment

Low.

@SyntaxColoring SyntaxColoring requested a review from a team as a code owner June 14, 2024 00:26
@SyntaxColoring SyntaxColoring changed the base branch from edge to recovery_for_all June 14, 2024 00:40
@SyntaxColoring SyntaxColoring requested a review from a team June 14, 2024 00:41
@SyntaxColoring SyntaxColoring added api Affects the `api` project protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs labels Jun 14, 2024
@SyntaxColoring SyntaxColoring changed the title refactor(api): Emit proper comment commands from legacy protocols. refactor(api): Emit proper comment commands from legacy protocols Jun 14, 2024
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 correct to me!

Copy link
Contributor

@TamarZanzouri TamarZanzouri left a comment

Choose a reason for hiding this comment

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

LGTM! thank you for picking this up!

@SyntaxColoring
Copy link
Contributor Author

Accidentally closed this. Replaced by PR #15426.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants