Skip to content
This repository has been archived by the owner on Oct 9, 2024. It is now read-only.

Fix stdio incompatibility with some third-party clients #604

Merged
merged 4 commits into from
Oct 23, 2023
Merged

Fix stdio incompatibility with some third-party clients #604

merged 4 commits into from
Oct 23, 2023

Conversation

JosBritton
Copy link
Contributor

Main issue ansible/vscode-ansible#1142

Does not interfere with third-party clients while still emitting logs for debugging.

Note: the method connection.console.debug does not exist. If i recall correctly console.debug is the same console.log in node regardless.

@JosBritton
Stdio patch
8a1b6a9 
Do not log validation status on stdout, use connection logs instead.
See #540, ansible#541
@JosBritton
Fix: use optional chaining for connection as it may not exist for pur…
cc2654a 
…poses of testing.
@JosBritton JosBritton requested a review from a team as a code owner October 18, 2023 06:35
@JosBritton JosBritton temporarily deployed to ack October 18, 2023 06:35 — with GitHub Actions Inactive
priyamsahoo
priyamsahoo suggested changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@priyamsahoo priyamsahoo left a comment

Choose a reason for hiding this comment

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

While the PR looks good, making connection optional, I would still suggest to revert the replacement of debug() with log()

src/providers/validationProvider.ts Show resolved Hide resolved
src/providers/validationProvider.ts Show resolved Hide resolved
JosBritton
JosBritton commented Oct 20, 2023
View reviewed changes
Copy link
Contributor Author

@JosBritton JosBritton left a comment

Choose a reason for hiding this comment

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

connection does not support the changes requested, other implementations would have to happen elsewhere (connection.console.debug not existing, connection.console.log not accepting template literals).

priyamsahoo
priyamsahoo approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@priyamsahoo priyamsahoo left a comment

Choose a reason for hiding this comment

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

Based on the explanation, the PR looks good to me.
Thanks for the changes.

@priyamsahoo priyamsahoo temporarily deployed to ack October 23, 2023 07:02 — with GitHub Actions Inactive
@priyamsahoo priyamsahoo added the bug Something isn't working label Oct 23, 2023
@priyamsahoo priyamsahoo temporarily deployed to ack October 23, 2023 07:03 — with GitHub Actions Inactive
@priyamsahoo priyamsahoo temporarily deployed to ack October 23, 2023 12:59 — with GitHub Actions Inactive
@priyamsahoo priyamsahoo merged commit fad31c1 into ansible:main Oct 23, 2023
9 checks passed
@JosBritton JosBritton deleted the stdoutfix branch October 23, 2023 20:29
@AmandaCameron AmandaCameron mentioned this pull request Dec 3, 2023
Open
@fredericgiquel fredericgiquel mentioned this pull request Jan 18, 2024
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@priyamsahoo priyamsahoo priyamsahoo approved these changes

@cristianonicolai cristianonicolai Awaiting requested review from cristianonicolai cristianonicolai is a code owner automatically assigned from ansible/devtools

@audgirka audgirka Awaiting requested review from audgirka audgirka is a code owner automatically assigned from ansible/devtools

Assignees
No one assigned
Labels
bug Something isn't working
Projects
🧰 devtools project board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JosBritton @priyamsahoo