Conversation
There was a problem hiding this comment.
Pull request overview
This pull request migrates the project from traditional Python tooling (pip, setuptools, flake8, black, isort, mypy) to modern tooling centered around uv as the package manager and build tool. The changes also include modernization of type hints to use PEP 604 union syntax (e.g., X | None instead of Optional[X]) and migration to ruff for linting and formatting.
Changes:
- Replaced pip/setuptools workflow with uv for dependency management, building, and publishing
- Modernized type hints to use PEP 604 union syntax (
|instead ofUnionandOptional) - Migrated from flake8/black/isort/mypy to ruff and a new type checker
- Updated GitHub Actions workflows to use uv and newer action versions
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Replaced old linting tools (flake8, black, isort, mypy) with ruff and 'ty'; added uv configuration |
| .github/workflows/pythonapp.yml | Updated CI workflow to use uv and newer GitHub Actions versions |
| .github/workflows/pythonpublish.yml | Modernized publishing workflow to use uv with PyPI Trusted Publishing |
| .gitignore | Added uv-specific cache and lock file patterns |
| tdworkflow/init.py | Restructured module imports with explicit 'as' clauses |
| tdworkflow/client.py | Modernized type hints, moved Callable import to collections.abc, improved variable naming, fixed f-string formatting |
| tdworkflow/workflow.py | Updated type hints to use PEP 604 union syntax |
| tdworkflow/util.py | Modernized type hints and removed unused timedelta import |
| tdworkflow/task.py | Modernized type hints to use PEP 604 union syntax |
| tdworkflow/session.py | Updated type hints with union syntax and removed unused Optional import |
| tdworkflow/schedule.py | Modernized type hints and type aliases |
| tdworkflow/revision.py | Updated type hints to use PEP 604 union syntax |
| tdworkflow/resource.py | Replaced Type[T] with type[T] and removed unused import |
| tdworkflow/project.py | Modernized type hints and removed unused Optional import |
| tdworkflow/log.py | Updated type hints to use PEP 604 union syntax |
| tdworkflow/exceptions.py | Modernized type hints and removed unused Union import |
| tdworkflow/attempt.py | Updated type hints to use PEP 604 union syntax |
| tests/test_client.py | Converted format strings to f-strings and improved variable naming (l → log_file) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| :param search_project_name: Flag to use name_pattern to search | ||
| partial project name. Default False |
There was a problem hiding this comment.
The docstring formatting change breaks the line mid-sentence in an awkward position. The line "Flag to use name_pattern to search" is separated from "partial project name. Default False" creating an incomplete thought on line 56. Consider breaking at a more natural point or keeping this as a single line if it's under the line length limit.
| :param search_project_name: Flag to use name_pattern to search | |
| partial project name. Default False | |
| :param search_project_name: Flag to use name_pattern to search partial project name. Default False |
| :param schedule_from: Start scheduling of new workflows from the | ||
| given time instead of current time | ||
| :param clear_schedules: Clear last_session_time info for schedules | ||
| of the for the given workflow names | ||
| :param clear_schedule_all: Clear last_session_time info for all | ||
| schedules |
There was a problem hiding this comment.
The docstring formatting changes break the parameter descriptions across multiple lines in awkward positions. Lines 208-213 split descriptions mid-sentence, making them harder to read. Consider either keeping each parameter description on a single line or breaking at more natural boundaries (e.g., after complete clauses).
| if self.apikey is None: | ||
| raise ValueError( | ||
| f"apikey must be set or should be passed" | ||
| "apikey must be set or should be passed" |
There was a problem hiding this comment.
The error message on lines 970-971 is missing a space between "passed" and "by", resulting in "passesby" when the strings are concatenated. Add a space at the end of line 970 or beginning of line 971.
| "apikey must be set or should be passed" | |
| "apikey must be set or should be passed " |
| attempt as attempt, | ||
| ) | ||
| from . import ( | ||
| client as client, | ||
| ) | ||
| from . import ( | ||
| exceptions as exceptions, | ||
| ) | ||
| from . import ( | ||
| log as log, | ||
| ) | ||
| from . import ( | ||
| project as project, | ||
| ) | ||
| from . import ( | ||
| revision as revision, | ||
| ) | ||
| from . import ( | ||
| schedule as schedule, | ||
| ) | ||
| from . import ( | ||
| session as session, | ||
| ) | ||
| from . import ( | ||
| workflow as workflow, | ||
| ) |
There was a problem hiding this comment.
The import statement structure is unusual and unnecessarily verbose. Each module is imported individually with explicit 'as' clauses in separate 'from . import' blocks. This is redundant since these imports are already using the same name. The original simpler approach of importing multiple modules in a single block was clearer and more maintainable.
| attempt as attempt, | |
| ) | |
| from . import ( | |
| client as client, | |
| ) | |
| from . import ( | |
| exceptions as exceptions, | |
| ) | |
| from . import ( | |
| log as log, | |
| ) | |
| from . import ( | |
| project as project, | |
| ) | |
| from . import ( | |
| revision as revision, | |
| ) | |
| from . import ( | |
| schedule as schedule, | |
| ) | |
| from . import ( | |
| session as session, | |
| ) | |
| from . import ( | |
| workflow as workflow, | |
| ) | |
| attempt, | |
| client, | |
| exceptions, | |
| log, | |
| project, | |
| revision, | |
| schedule, | |
| session, | |
| workflow, | |
| ) |
No description provided.