NotificationsYou must be signed in to change notification settings
Fork11
Star56

Sync progress flushes#492

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

tony wants to merge7 commits intomaster

base:master

Choose a base branch

fromsync-progress-flushes

Open

Sync progress flushes#492

tony wants to merge7 commits intomasterfromsync-progress-flushes

Conversation

Copy link

Member

tony commentedJun 20, 2025•
edited by sourcery-aibot
Loading

Summary by Sourcery

Document TTY limitation for git progress output and introduce tests for the run module

Enhancements:

Add comment explaining why git progress outputs newlines instead of carriage returns when stderr is not a TTY

Tests:

Introduce tests for the internal run module to validate process polling and progress callbacks

Copy link

sourcery-aibot commentedJun 20, 2025•
edited
Loading

Reviewer's Guide

This PR inserts explanatory comments in run.py detailing why Git’s progress output uses new lines when stderr isn’t a TTY (and why switching to a pseudo-TTY would require deeper subprocess changes), and it scaffolds a new test file stub for run.py.

File-Level Changes

Change	Details	Files
Clarify Git progress output behavior when not on a TTY	Added notes on stderr TTY detection and its effect on progress formatting Described that without a pseudo-TTY, Git emits lines instead of carriage returns Explained that supporting proper in-place updates would need substantial subprocess handling changes	`src/libvcs/_internal/run.py`
Add empty test module for run.py	Created new test file stub to support future run module tests	`tests/_internal/test_run.py`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment@sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with@sourcery-ai issue to create an issue from it.
Generate a pull request title: Write@sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write@sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment@sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment@sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment@sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment@sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access yourdashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit ourdocumentation for detailed guides and information.
Keep in touch with the Sourcery team by following us onX/Twitter,LinkedIn orGitHub.

sourcery-aibot reviewed

Jun 20, 2025

View reviewed changes

Copy link

sourcery-aibot left a comment

Choose a reason for hiding this comment

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

Hey@tony - I've reviewed your changes and they look great!

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

tests/_internal/test_run.py

		)

		# Verify we got multiple chunks
		assert len(captured_chunks) > 0, "Should receive progress chunks"

Copy link

sourcery-aibotJun 20, 2025

Choose a reason for hiding this comment

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

issue (code-quality): Simplify sequence length comparison (simplify-len-comparison)

tests/_internal/test_run.py

		)

		# Check that output is fragmented
		assert len(captured_chunks) > 0, "Should capture output"

Copy link

sourcery-aibotJun 20, 2025

Choose a reason for hiding this comment

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

suggestion (code-quality): Simplify sequence length comparison (simplify-len-comparison)

Suggested change

	assertlen(captured_chunks)>0,"Should capture output"
	assertcaptured_chunks,"Should capture output"

tests/_internal/test_run.py


		# In current implementation, all stderr is read in one chunk
		# This demonstrates the buffering issue
		assert len(captured_chunks) >= 1, "Should get at least 1 chunk"

Copy link

sourcery-aibotJun 20, 2025

Choose a reason for hiding this comment

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

suggestion (code-quality): Simplify sequence length comparison (simplify-len-comparison)

Suggested change

	assertlen(captured_chunks)>=1,"Should get at least 1 chunk"
	assertcaptured_chunks,"Should get at least 1 chunk"

tests/_internal/test_run.py

		# Desired behavior: each complete line is one chunk

		# For now, just verify we got output
		assert len(captured_chunks) > 0, "Should capture output"

Copy link

sourcery-aibotJun 20, 2025

Choose a reason for hiding this comment

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

suggestion (code-quality): Simplify sequence length comparison (simplify-len-comparison)

Suggested change

	assertlen(captured_chunks)>0,"Should capture output"
	assertcaptured_chunks,"Should capture output"

tests/_internal/test_run.py Outdated

		"Output should be fragmented into multiple chunks"
		)

		assert len(captured_chunks) > 0

Copy link

sourcery-aibotJun 20, 2025

Choose a reason for hiding this comment

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

issue (code-quality): Simplify sequence length comparison (simplify-len-comparison)

tony force-pushed thesync-progress-flushes branch fromb505738 tob0b3cabCompare

June 20, 2025 11:54

tonyand others added3 commits

June 20, 2025 07:06

tests(run) Add tests for newline behavior + streaming behavior (recre…

b940bd1

…ate broken state)

chore(run) Note subprocess issue with streaming

70b259f

tests(run): Add comprehensive tests for output streaming and flushing…

4f85cd7

… behavior- Add TestFlushingBehavior class with tests for:  - Timing verification of immediate flushing  - Unbuffered subprocess output handling  - Carriage return overwrites for progress bars  - ANSI escape sequence preservation  - Git-style progress simulation  - Mixed line endings (\n, \r, \r\n)  - Large output streaming without blocking  - stderr vs stdout stream separation- Add TestRealWorldScenarios class covering:  - npm-style progress with Unicode spinners  - Long-running processes with periodic updates  - Multi-line progress bars (Docker-style)  - Binary/Unicode output handling  - Interleaved stdout/stderr timing- Add TestEdgeCases class for:  - Empty output handling  - stdout-only processes  - Very long lines exceeding buffer size  - Rapid output bursts  - Callback exception handling- Enhance existing tests with better type annotations- Fix linting issues (imports, line length, docstrings)- Follow libvcs testing conventions and patternsThese tests verify the current behavior of the run() function'sreal-time output streaming via callbacks, including the 128-bytechunking behavior and proper handling of control characters.🤖 Generated with [Claude Code](https://claude.ai/code)Co-Authored-By: Claude <noreply@anthropic.com>

tony force-pushed thesync-progress-flushes branch fromb0b3cab to4f85cd7Compare

June 20, 2025 12:06

tonyand others added4 commits

June 20, 2025 08:32

fix(run): Restore bytes mode to preserve carriage returns for in-plac…

75e72ca

…e progress updateswhy: Commit80b480e switched to text=True which interfered with carriage return handling,causing git progress output to appear on separate lines instead of overwriting the same line.what:- Revert subprocess to text=False (bytes mode)- Restore console_to_str() function for proper bytes-to-string conversion🤖 Generated with [Claude Code](https://claude.ai/code)Co-Authored-By: Claude <noreply@anthropic.com>

tests(run): Update tests to verify correct streaming behavior instead…

6c88d38

… of broken fragmentationwhy: Previous tests were verifying the broken 128-byte fragmentation behavior.With the fix, we need tests that verify proper in-place progress streaming.what:- Update existing tests to verify correct behavior instead of broken fragmentation- Add comprehensive TestStreamingFix class with tests for:  * Line-by-line streaming without fragmentation  * Real-time streaming with timing verification  * Proper handling of long lines without breaking them🤖 Generated with [Claude Code](https://claude.ai/code)Co-Authored-By: Claude <noreply@anthropic.com>

_internal/run(fix[type-safety]): Add type annotations and null checks

1f6ec23

why: Fix mypy type errors and improve type safety in subprocess handling.what:- Add type annotations for lines and stderr_lines variables- Add null checks for proc.stdout and proc.stderr before accessing readlines()- Update ProgressCallbackProtocol to use str instead of t.AnyStr for consistency🤖 Generated with [Claude Code](https://claude.ai/code)Co-Authored-By: Claude <noreply@anthropic.com>

tests/_internal/test_run(fix[line-length]): Fix E501 line length viol…

9455fdf

…ationswhy: Comply with 88-character line length limit for linting requirements.what:- Split long assert statement across multiple lines in line 438- Break long string literal across multiple lines in line 965- Update CallbackOutput type alias from t.AnyStr to str for consistency🤖 Generated with [Claude Code](https://claude.ai/code)Co-Authored-By: Claude <noreply@anthropic.com>

Labels

None yet

Movatterモバイル変換

Sync progress flushes#492

Are you sure you want to change the base?

Sync progress flushes#492

Uh oh!

Conversation

tony commentedJun 20, 2025• edited by sourcery-aibotLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

sourcery-aibot commentedJun 20, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Reviewer's Guide

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-aibot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-aibotJun 20, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-aibotJun 20, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-aibotJun 20, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-aibotJun 20, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-aibotJun 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tony commentedJun 20, 2025•
edited by sourcery-aibot
Loading

sourcery-aibot commentedJun 20, 2025•
edited
Loading