Repository navigation
fix(core): emit LF newlines for tsv output - #33427
Open
Rohan Patnaik (rohan-patnaik) wants to merge 2 commits into
Open
Rohan Patnaik (rohan-patnaik) wants to merge 2 commits into
Rohan Patnaik (rohan-patnaik) wants to merge 2 commits into
Conversation
|
Validation for Azure CLI Full Test Starting...
Thanks for your contribution! |
|
Hi Rohan Patnaik (@rohan-patnaik), |
|
Validation for Breaking Change Starting...
Thanks for your contribution! |
microsoft-github-policy-service
Bot
added
the
customer-reported
Contributor
|
Thank you for your contribution Rohan Patnaik (@rohan-patnaik)! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
Bot
added
Auto-Assign
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a TSV-specific behavior to force LF (\n) newlines during output, and introduces a regression test to ensure TSV output remains consistent across platforms (notably Windows).
Changes:
- Add a regression test validating TSV output uses LF newlines even when the stream is configured for CRLF.
- Override
AzOutputProducer.out()to reconfigure the output stream newline handling when output format istsv.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/azure-cli-core/azure/cli/core/tests/test_output.py |
Adds regression test asserting TSV output is LF-terminated. |
src/azure-cli-core/azure/cli/core/_output.py |
Reconfigures output stream newline handling for TSV via out() override. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+16
to
+21
| def out(self, obj, formatter=None, out_file=None): | ||
| if get_output_format(self.cli_ctx) == "tsv": | ||
| file = out_file or sys.stdout | ||
| if hasattr(file, "reconfigure"): | ||
| file.reconfigure(newline="\n") | ||
| super().out(obj, formatter=formatter, out_file=out_file) |
Contributor
Author
There was a problem hiding this comment.
Addressed in d523bf1: out() now returns super().out(...) and passes the reconfigured stream through as out_file=file.
Collaborator
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
Member
Live test results —
|
Member
Live test results —
|
Member
Live test results —
|
Member
Live test results —
|
Aditya Pujara (a0x1ab)
left a comment
Aditya Pujara (a0x1ab)
left a comment
Member
There was a problem hiding this comment.
Review: fix(core): emit LF newlines for tsv output
CI Status ✅
All checks passed: license/cla, azdev-style, azdev-linter.
Live Test ⚠️
Live-test workflow dispatch failed to confirm a new run within the timeout window. Manual verification of TSV output on Windows is recommended before merging.
Code Review
src/azure-cli-core/azure/cli/core/_output.py
The approach is sound: overriding out() to call file.reconfigure(newline="\n") before delegating to the parent is the correct way to force LF newlines on Windows (where TextIOWrapper defaults to CRLF translation in text mode). The hasattr guard handles streams that don't support reconfigure gracefully.
One subtle concern: if out_file is None (i.e., sys.stdout), reconfigure is called on the process-wide stdout. This permanently changes stdout's newline mode for the rest of the process after the first TSV command. For a CLI that exits after each invocation this is fine, but worth noting.
src/azure-cli-core/azure/cli/core/tests/test_output.py
The test is well-structured: it sets up a TextIOWrapper with newline='\r\n' to simulate Windows stdout, then asserts the buffer contains only \n separators. This directly validates the fix.
Summary
The fix is minimal, targeted, and well-tested. No functional issues found. Copilot please ensure a live test on Windows confirms TSV output piping works as expected before merging.
Posted by agent-assist (autonomous bug-fix pipeline).
Member
Live test results —
|
Member
Live test results —
|
Contributor
Author
|
Jiashuo Li (@jiasli) Ethan Yang (@necusjz), could this backlog PR be considered for an upcoming milestone? Happy to refresh it or provide anything else needed for review. Thanks! |
3 tasks done
Contributor
Author
|
The fix for #33385 landed on dev in #34236 today (42381af). It covers all stdout formats and lets users select native or LF newlines. Keeping this PR’s unconditional TSV override would bypass an explicit core.output_newline=native setting, so I’m leaving this branch unchanged pending maintainer guidance on whether any separate gap remains. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #33385
Summary
This forces
tsvoutput streams to use LF newlines before writing the formatted result.Why
On Windows, Python text-mode output can translate
\ninto\r\n. That makesaz ... -o tsvunsafe in bash-style command substitution because intermediate values keep a trailing\r, which can then break follow-up CLI calls.Test plan
.venv/bin/python -m pytest src/azure-cli-core/azure/cli/core/tests/test_output.py.venv/bin/python -m compileall -q src/azure-cli-core/azure/cli/core/_output.py src/azure-cli-core/azure/cli/core/tests/test_output.pygit diff --check