Feature/test branch #3
Conversation
Reviewer's Guide by SourceryThis PR implements a comprehensive Python virtual environment creation tool with improved CI/CD workflow. The changes include a complete rewrite of the virtual environment creator with proper class structure, type hints, and extensive test coverage. The CI workflow has been enhanced with separate jobs for testing, linting, and security checks. Class diagram for VenvCreatorclassDiagram
class VenvCreator {
-String os_type
-bool is_windows
-bool is_linux
-List~String~ python_installations
-String username
-Optional~String~ dependencies_folder
+set_dependencies_folder(folder_path: String) bool
+find_dependencies() List~String~
+install_local_dependencies(venv_path: String) Optional~bool~
+get_current_username() Optional~String~
+_get_pip_path(venv_path: String) String
+find_python_installations() List~String~
+_find_windows_python() List~String~
+_find_linux_python() List~String~
+get_python_version(python_path: String) String
+create_venv(python_path: String, venv_path: String) bool
+install_requirements(venv_path: String, requirements_path: Optional~String~) Optional~bool~
+get_project_details() Dict~String, String~
+print_activation_instructions(project_path: String, venv_name: String) void
+run() void
}
Class diagram for TestVenvCreatorclassDiagram
class TestVenvCreator {
-VenvCreator venv_creator
-String temp_dir
-String test_requirements
-String test_dependencies
+setUp() void
+tearDown() void
+create_test_wheel(name: String) String
+test_init() void
+test_set_dependencies_folder() void
+test_find_dependencies() void
+test_install_local_dependencies() void
+test_get_python_version() void
+test_create_venv() void
+test_get_project_details() void
+test_install_requirements() void
+test_find_python_installations() void
+test_print_activation_instructions() void
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @selectdimensions - I've reviewed your changes - here's some feedback:
Overall Comments:
- The security job is missing the bandit security scan that was present in the previous version. Consider adding back
bandit -r src teststo scan Python files for security issues.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟡 Security: 1 issue found
- 🟡 Testing: 5 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| else: | ||
| temp.write(f"{line}\n") | ||
|
|
||
| if missing_files: |
There was a problem hiding this comment.
suggestion (bug_risk): Consider failing fast when missing dependencies are detected rather than continuing with an incomplete installation
if missing_files:
print("\nError: Missing required dependency files:")
for file in missing_files:
print(f" - {file}")
sys.exit(1)| for dep in dependencies: | ||
| dep_name = os.path.basename(dep) | ||
| print(f"Installing {dep_name}") | ||
| result = subprocess.run( # nosec B603 |
There was a problem hiding this comment.
🚨 issue (security): Add path sanitization to prevent command injection risks in subprocess calls
Consider using shlex.quote or similar to sanitize paths before passing them to subprocess.run
|
|
||
| dependency_files = [] | ||
| extensions = ['.whl', '.tar.gz', '.zip'] | ||
| for ext in extensions: |
There was a problem hiding this comment.
suggestion (performance): Optimize file pattern matching by using a single glob operation with multiple patterns
Use glob.glob(os.path.join(self.dependencies_folder, '*.{whl,tar.gz,zip}'), recursive=False) for better performance
dependency_files = glob.glob(
os.path.join(self.dependencies_folder, '*.{whl,tar.gz,zip}'))| version = self.get_python_version(path) | ||
| try: | ||
| version_numbers = version.replace("Python ", "").split(".") | ||
| version_tuple = tuple(int(num) for num in version_numbers) |
There was a problem hiding this comment.
issue: Version parsing in _find_windows_python doesn't handle pre-release versions correctly
Consider using packaging.version.parse for robust version comparison that handles alpha/beta releases
| missing_files.append(line) | ||
| print(f"Warning: Required file not found: {line}") | ||
| print(f"Expected location: {absolute_path}") | ||
| elif os.path.getsize(absolute_path) == 0: |
There was a problem hiding this comment.
suggestion: File validation should go beyond just checking for non-zero size
Consider adding basic format validation for wheel files and other package formats
elif os.path.getsize(absolute_path) == 0 or not _is_valid_package_format(absolute_path):| from typing import List, Dict, Optional, Tuple | ||
|
|
||
|
|
||
| class VenvCreator: |
There was a problem hiding this comment.
issue (complexity): Consider separating the user interface code from the core virtual environment creation logic into distinct classes
The VenvCreator class mixes UI concerns with core functionality, making it harder to test and maintain. Consider extracting UI interactions into a separate class:
class VenvCreatorCLI:
def __init__(self, venv_creator: VenvCreator):
self.venv_creator = venv_creator
def get_project_details(self) -> Dict[str, str]:
"""Get project details via CLI"""
details: Dict[str, str] = {}
details['name'] = input("\nEnter project name: ").strip()
details['path'] = self._get_valid_project_path()
details['venv_name'] = self._get_venv_name()
if self._confirm_local_dependencies():
details['dependencies_path'] = self._get_dependencies_path()
details['requirements_path'] = self._handle_requirements(details['path'])
return details
class VenvCreator:
def create_project(self, details: Dict[str, str]) -> bool:
"""Core logic for creating project - no UI interaction"""
venv_path = os.path.join(details['path'], details['venv_name'])
if not self._ensure_venv_path(venv_path):
return False
if not self.create_venv(self.selected_python, venv_path):
return False
if 'dependencies_path' in details:
self.set_dependencies_folder(details['dependencies_path'])
self.install_local_dependencies(venv_path)
if 'requirements_path' in details:
self.install_requirements(venv_path, details['requirements_path'])
return TrueThis separation:
- Makes the code more testable - core logic can be tested without UI
- Allows adding new interfaces (e.g., GUI) without changing core logic
- Reduces complexity by giving each class a single responsibility
- Makes the code more maintainable by isolating UI-related changes
| if installations: | ||
| self.assertTrue(all(os.path.exists(path) for path in installations)) |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| temp_requirements = os.path.join(project_root, 'temp_requirements.txt') | ||
|
|
||
| try: | ||
| missing_files = [] |
There was a problem hiding this comment.
issue (code-quality): Extract code out into method (extract-method)
|
|
||
| def get_project_details(self) -> Dict[str, str]: | ||
| """Get project details from user""" | ||
| project_details: Dict[str, str] = {} |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Merge dictionary assignment with declaration (
merge-dict-assign) - Use named expression to simplify assignment and conditional [×2] (
use-named-expression) - Lift code into else after jump in control flow (
reintroduce-else) - Swap if/else branches (
swap-if-else-branches)
| result = self.install_local_dependencies(venv_path) | ||
| if result: | ||
| print("Local dependencies installed successfully!") | ||
| elif result is False: | ||
| print("Failed to install local dependencies.") | ||
|
|
||
| if 'requirements_path' in project_details: | ||
| result = self.install_requirements(venv_path, project_details['requirements_path']) |
There was a problem hiding this comment.
issue (code-quality): Use named expression to simplify assignment and conditional [×2] (use-named-expression)
Summary by Sourcery
Add a new script for virtual environment management and update CI workflows to enhance testing and linting processes.
New Features:
CI:
Tests: