Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors SSH config utilities from a class-based approach to standalone functions, updates tests accordingly, and transitions packaging from setup.py to pyproject.toml with a version bump.
- Refactor
ConfigPathUtilitymethods into module-level functions (get_config_content,get_config_file_row_data,get_config_file_host_data) and add port parsing. - Update tests to mock file I/O for the new APIs and add error-case coverage.
- Remove
setup.py, bump version to 3.1.0, and switch to editable installs in CI and README.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_utils.py | Adapt tests for function-based utilities and add missing error test. |
| src/bitssh/utils.py | Replace class methods with functions, add port parsing, remove unused imports. |
| src/bitssh/ui.py | Call new get_config_file_row_data instead of old class. |
| src/bitssh/prompt.py | Switch to subprocess.run, use new host-data function, improve error handling. |
| src/bitssh/cli.py | Remove unused imports. |
| src/bitssh/argument_parser.py | Add -v short flag for version output. |
| src/bitssh/init.py | Bump package version to 3.1.0. |
| setup.py | Remove legacy file. |
| pyproject.toml | Bump version to 3.1.0, update build requirements, leave path dependency. |
| README.md | Change install command to pip install -e .. |
| .github/workflows/test.yml | Use editable install in CI. |
Comments suppressed due to low confidence (4)
src/bitssh/prompt.py:31
- [nitpick] Avoid naming the exception variable
Error, which shadows the built-in. Use a lowercase name likeerrorefor clarity.
except Exception as Error:
src/bitssh/ui.py:16
- [nitpick] The loop variable
iis not descriptive. Consider renaming it to something likeroworentryto improve readability.
for i in get_config_file_row_data():
tests/test_utils.py:13
- [nitpick] The test class name still references
ConfigPathUtilityeven though you're now testing standalone functions. Consider renaming it to something likeTestUtilsFunctionsto reflect the current APIs.
class TestConfigPathUtility(unittest.TestCase):
pyproject.toml:28
- The
pathdependency is no longer used inutils.py(import removed). Consider removing it frompyproject.tomlto avoid unnecessary dependencies.
"path>=16.9.0",
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to a function-based approach for handling SSH configuration, along with updating tests and documentation. Key changes include:
- Removing the ConfigPathUtility class in favor of standalone functions in utils.py.
- Updating tests to use the new function-based API and adding error handling tests.
- Bumping version numbers and updating packaging scripts and workflows.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_utils.py | Refactored tests to use functions get_config_content, get_config_file_row_data, and get_config_file_host_data. |
| src/bitssh/utils.py | Replaced the class implementation with function-based utilities and added port extraction. |
| src/bitssh/ui.py | Updated UI code to use the new functions for retrieving config rows. |
| src/bitssh/prompt.py | Switched to subprocess for command execution and added error handling for answer parsing. |
| src/bitssh/argument_parser.py | Added a short flag for version output. |
| src/bitssh/init.py | Updated the version to 3.1.0. |
| setup.py | Removed as part of migration. |
| pyproject.toml | Updated version and dependency versions. |
| README.md | Updated installation instructions. |
| .github/workflows/* | Updated CI configurations and added a Dockerhub publish workflow. |
Comments suppressed due to low confidence (2)
tests/test_utils.py:13
- [nitpick] The test class name 'TestConfigPathUtility' is now misleading since the code has been refactored to use function-based utilities. Consider renaming it (e.g., 'TestConfigFunctions') to reflect the current implementation.
class TestConfigPathUtility(unittest.TestCase):
src/bitssh/prompt.py:21
- [nitpick] The variable name 'cmd' is ambiguous as it holds the selected host information rather than a complete command. Consider renaming it to 'selected_host' or similar for clarity.
cmd: str = answers
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the class-based config parser into standalone functions, adds tests (including a file-not-found case), and removes setup.py in favor of a pyproject.toml-driven build.
- Refactored
ConfigPathUtilitymethods into module-level functions and added port parsing - Updated tests to target the new functions and added a missing-file error test
- Removed
setup.py, bumped version to 3.1.0, updated dependencies, and adjusted CI/workflows
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_utils.py | Updated to test new functions, added file-not-found error test |
| src/bitssh/utils.py | Converted class methods to functions; added port regex parsing |
| src/bitssh/ui.py | Switched to get_config_file_row_data() and cleaned up imports |
| src/bitssh/prompt.py | Replaced os.system calls with subprocess, updated imports |
| src/bitssh/cli.py | Removed unused imports |
| src/bitssh/argument_parser.py | Added -v alias for version flag |
| src/bitssh/init.py | Bumped __version__ to "3.1.0" |
| setup.py | Removed (migrated to pyproject.toml) |
| pyproject.toml | Bumped version to 3.1.0, updated dependency bounds |
| README.md | Updated install command to editable mode |
| .github/workflows/test.yml | Changed install to editable install |
| .github/workflows/publish_dockerhub.yml | Added DockerHub publish pipeline on release |
Comments suppressed due to low confidence (7)
src/bitssh/ui.py:16
- [nitpick] The loop variable
iis ambiguous. Rename it to something more descriptive likeroworhost_entry.
for i in get_config_file_row_data():
tests/test_utils.py:13
- [nitpick] The test class name references
ConfigPathUtility, which was removed. Consider renaming to reflect the new functional API, e.g.,TestUtilsFunctions.
class TestConfigPathUtility(unittest.TestCase):
pyproject.toml:31
- The
pathdependency is no longer used after removingfrom path import Path—consider removing it frompyproject.toml.
"path>=16.9.0",
src/bitssh/prompt.py:26
- The module no longer imports
osbut still referencesos.name, causing aNameError. Please addimport osat the top.
if os.name == "nt": # Windows
src/bitssh/prompt.py:27
- When using
shell=True, pass a string command instead of a list; e.g.,subprocess.run("cls", shell=True, check=True)to avoid unexpected behavior.
subprocess.run(["cls"], shell=True, check=True)
src/bitssh/utils.py:16
- Add a return type annotation (e.g.,
-> Dict[str, Dict[str, str]]) and a docstring to clarify the function’s behavior and return value.
def get_config_content():
src/bitssh/argument_parser.py:11
- [nitpick] For consistency with common CLI conventions, place the short flag before the long flag:
parser.add_argument('-v', '--version', ...).
"-v",
Description
This PR add function based approach and add tests for it and removed
setup.pyFixes: #23 #4