Conversation
lord63
commented
Mar 21, 2026
- drop python 2.x support,add 3.10+ support;
- add context manager support, thanks to @liiight ;
- migrate to uv and github aciton;
To be able to run the test prior to installation, as part of building a distro package.
- Add spinner reentry context manager test - Parametrize non-TTY stdout tests
Add context manager support
…str() Co-authored-by: lord63 <5268051+lord63@users.noreply.github.com> Agent-Logs-Url: https://github.com/lord63/py-spin/sessions/d2ede27f-2a5b-4c50-8efc-1cdfb90920d3
There was a problem hiding this comment.
Pull request overview
This PR modernizes the project for a 1.2.0 release by dropping legacy Python packaging/CI tooling, adding Python 3.10+ support, and introducing context-manager support for the spinner.
Changes:
- Migrate packaging to
pyproject.toml(Hatchling) and adduv.lockfor reproducible dev installs. - Replace Travis/tox/dev-requirements with a GitHub Actions workflow using
uv+ruff+pytest. - Add
Spinnercontext-manager support and expand the test suite to cover TTY/non-TTY behavior.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Adds uv lockfile to pin dev tool/test dependencies. |
pyproject.toml |
Introduces modern packaging metadata + Python >=3.10. |
.github/workflows/ci.yml |
Adds CI using uv/ruff/pytest across Python 3.10–3.13. |
pyspin/spin.py |
Removes Py2 compat, adds TTY detection + context manager, refactors spinner output behavior. |
test_pyspin.py |
Adds tests for context manager, non-TTY suppression, and flushing behavior. |
README.rst |
Documents context-manager usage. |
MANIFEST.in |
Updates included files/excludes (but needs correction). |
tox.ini |
Removed (legacy test runner). |
.travis.yml |
Removed (legacy CI). |
setup.py / setup.cfg / dev-requirements.txt |
Removed (legacy packaging/dev deps). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MANIFEST.in
Outdated
| @@ -1,2 +1,4 @@ | |||
| include README.md | |||
There was a problem hiding this comment.
MANIFEST.in includes README.md, but this repository uses README.rst (and pyproject.toml also points to README.rst). Including a non-existent file can break/warn during source distribution builds; update the manifest to include the correct README file (or remove MANIFEST.in if it’s no longer used with Hatchling).
| include README.md | |
| include README.rst |
README.rst
Outdated
| from __future__ import print_function | ||
|
|
||
| import time | ||
|
|
||
| from pyspin.spin import Default, Spinner |
There was a problem hiding this comment.
The new context-manager example still includes from __future__ import print_function, but the project now requires Python >= 3.10. This import is unnecessary in supported versions and may confuse readers about supported Python versions; consider removing it from the added snippet.
pyspin/spin.py
Outdated
| def _start(self): | ||
| if _is_tty(sys.stdout): | ||
| self._stop_event = threading.Event() | ||
| self.spin_thread = threading.Thread(target=self._init_spin) |
There was a problem hiding this comment.
The spinner thread is created as a non-daemon thread. If a user starts a spinner and exits unexpectedly without calling __exit__/_stop(), this thread can keep the process alive. Consider setting daemon=True (and optionally a thread name) when constructing threading.Thread(...) to avoid hanging process shutdowns.
| self.spin_thread = threading.Thread(target=self._init_spin) | |
| self.spin_thread = threading.Thread( | |
| target=self._init_spin, | |
| daemon=True, | |
| name="SpinnerThread", | |
| ) |
pyproject.toml
Outdated
| @@ -0,0 +1,34 @@ | |||
| [project] | |||
| name = "pyspin" | |||
| version = "1.1.1" | |||
There was a problem hiding this comment.
The project is labeled as a "Release 1.2.0" in the PR metadata, but the packaged version here is still 1.1.1. If this PR is intended to publish 1.2.0, bump the version accordingly (and keep uv.lock / any other version references consistent).
… exit Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
ChangLog.md:9
- The changelog mentions hardened TTY detection, but the new behavior also suppresses all spinner output (including custom
endinglike "DONE") when stdout isn’t a TTY. Since that’s a user-visible/breaking behavior change, it would help to call it out explicitly in this 1.2.0 entry (e.g., underchange).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pyspin/spin.py
Outdated
| sys.stdout.flush() | ||
| time.sleep(0.1) | ||
| if is_tty: | ||
| while future.running(): |
There was a problem hiding this comment.
make_spin drives the spinner loop with while future.running(). A concurrent.futures.Future can be in the PENDING state immediately after submit(), where running() is false even though the work hasn’t completed yet, causing the loop to skip and the ending to print before the wrapped function finishes. Use while not future.done() (or while not future.done(): ...) to reliably spin until completion.
| while future.running(): | |
| while not future.done(): |
Makefile
Outdated
| @py.test --pep8 -v test_pyspin.py pyspin/ | ||
|
|
There was a problem hiding this comment.
The test target still runs py.test --pep8, but this PR removes pytest-pep8 (and CI now uses pytest + ruff). As-is, make test will fail due to the unknown --pep8 option. Update the target to run uv run pytest -v test_pyspin.py (and optionally add a separate lint target for uv run ruff check .).
| @py.test --pep8 -v test_pyspin.py pyspin/ | |
| @uv run pytest -v test_pyspin.py | |
| lint: | |
| @uv run ruff check . |
| def test_make_spin(): | ||
| @spin.make_spin(spin.Default, 'Downloading...') | ||
| def fake_download(): | ||
| time.sleep(2) | ||
|
|
||
| fake_download() | ||
|
|
||
|
|
||
| def test_make_spin_with_args(): | ||
| @spin.make_spin(spin.Default, 'Downloading...') | ||
| def fake_download(url, retry_times=3): | ||
| print("Downloading {0}, will retry {1} times".format(url, retry_times)) | ||
| time.sleep(2) | ||
|
|
||
| fake_download("https://www.example.com/text.txt", retry_times=5) |
There was a problem hiding this comment.
These tests still sleep for 2 seconds per case (and test_several_calls sleeps twice), which makes CI noticeably slower across the 4-version Python matrix. Consider reducing these waits (e.g., to a few hundred ms) or using a synchronization primitive (Event) so the spinner logic is exercised without long real-time delays.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
ChangLog.md:5
- There is trailing whitespace at the end of this line (after the contributor handle). Please remove it to avoid unnecessary diffs and keep markdown clean.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ("Box1", spin.Box1), | ||
| ("Box2", spin.Box2), | ||
| ("Box3", spin.Box3), | ||
| ("Box4", spin.Box4), | ||
| ("Box5", spin.Box5), | ||
| ("Box6", spin.Box6), | ||
| ("Box7", spin.Box7), | ||
| ("Spin1", spin.Spin1), | ||
| ("Spin2", spin.Spin2), | ||
| ("Spin3", spin.Spin3), | ||
| ("Spin4", spin.Spin4), | ||
| ("Spin5", spin.Spin5), | ||
| ("Spin6", spin.Spin6), | ||
| ("Spin7", spin.Spin7), | ||
| ("Spin8", spin.Spin8), | ||
| ("Spin9", spin.Spin9), |
There was a problem hiding this comment.
STYLES uses alignment spacing after commas (e.g., ("Box1", spin.Box1)), which will be flagged by ruff (E241: multiple spaces after ',') since CI runs ruff check .. Please remove the extra alignment spaces (or run an auto-formatter) so the examples pass linting.
| ("Box1", spin.Box1), | |
| ("Box2", spin.Box2), | |
| ("Box3", spin.Box3), | |
| ("Box4", spin.Box4), | |
| ("Box5", spin.Box5), | |
| ("Box6", spin.Box6), | |
| ("Box7", spin.Box7), | |
| ("Spin1", spin.Spin1), | |
| ("Spin2", spin.Spin2), | |
| ("Spin3", spin.Spin3), | |
| ("Spin4", spin.Spin4), | |
| ("Spin5", spin.Spin5), | |
| ("Spin6", spin.Spin6), | |
| ("Spin7", spin.Spin7), | |
| ("Spin8", spin.Spin8), | |
| ("Spin9", spin.Spin9), | |
| ("Box1", spin.Box1), | |
| ("Box2", spin.Box2), | |
| ("Box3", spin.Box3), | |
| ("Box4", spin.Box4), | |
| ("Box5", spin.Box5), | |
| ("Box6", spin.Box6), | |
| ("Box7", spin.Box7), | |
| ("Spin1", spin.Spin1), | |
| ("Spin2", spin.Spin2), | |
| ("Spin3", spin.Spin3), | |
| ("Spin4", spin.Spin4), | |
| ("Spin5", spin.Spin5), | |
| ("Spin6", spin.Spin6), | |
| ("Spin7", spin.Spin7), | |
| ("Spin8", spin.Spin8), | |
| ("Spin9", spin.Spin9), |
| def show(name, frames): | ||
| s = spin.Spinner(frames) | ||
| print(name) | ||
| for i in range(50): |
There was a problem hiding this comment.
The loop variable i is unused here; ruff/Pyflakes will flag this as F841 (assigned to but never used) in CI. Use _ (or otherwise use i) to avoid failing lint.
| for i in range(50): | |
| for _ in range(50): |