Feature/test branch #2
Conversation
Reviewer's Guide by SourceryThis PR implements a comprehensive Python virtual environment creator with improved CI/CD workflow. The changes include a complete restructuring of the project layout, enhanced testing infrastructure, and a robust virtual environment creation utility with cross-platform support. Class diagram for VenvCreatorclassDiagram
class VenvCreator {
-os_type: str
-is_windows: bool
-is_linux: bool
-python_installations: List~str~
-username: str
-dependencies_folder: Optional~str~
+set_dependencies_folder(folder_path: str) bool
+find_dependencies() List~str~
+install_local_dependencies(venv_path: str) Optional~bool~
+get_current_username() Optional~str~
+_get_pip_path(venv_path: str) str
+find_python_installations() List~str~
+_find_windows_python() List~str~
+_find_linux_python() List~str~
+get_python_version(python_path: str) str
+create_venv(python_path: str, venv_path: str) bool
+install_requirements(venv_path: str, requirements_path: Optional~str~) Optional~bool~
+get_project_details() Dict~str, str~
+print_activation_instructions(project_path: str, venv_name: str) void
+run() 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:
- Several docstring summaries are missing (marked with summary). Consider adding brief descriptions to help with code documentation.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟡 Testing: 1 issue 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.
| except (subprocess.SubprocessError, OSError): | ||
| return "Version unknown" | ||
|
|
||
| def create_venv(self, python_path: str, venv_path: str) -> bool: |
There was a problem hiding this comment.
🚨 suggestion (security): Add path validation before subprocess calls for security
Consider adding os.path.isfile() and os.access() checks on python_path before passing it to subprocess.run() to prevent potential command injection risks.
def create_venv(self, python_path: str, venv_path: str) -> bool:
if not os.path.isfile(python_path) or not os.access(python_path, os.X_OK):
return False| """Find Python installations based on OS""" | ||
| return self._find_windows_python() if self.is_windows else self._find_linux_python() | ||
|
|
||
| def _find_windows_python(self) -> List[str]: |
There was a problem hiding this comment.
suggestion: Improve version parsing error handling
Add explicit error handling for cases where Python version string doesn't match expected format, rather than silently continuing.
def _find_windows_python(self) -> List[str]:
"""Find Python installations on Windows and validate version strings.
Raises:
ValueError: If a Python installation has an invalid version string format
"""| if os.path.exists(temp_requirements): | ||
| os.remove(temp_requirements) | ||
|
|
||
| def get_project_details(self) -> Dict[str, str]: |
There was a problem hiding this comment.
suggestion: Split get_project_details into smaller focused methods
Consider breaking this method into smaller methods like get_project_name(), get_project_path(), get_venv_name() etc. for better maintainability.
def get_project_name(self) -> str:
"""Get project name from user"""
return self._get_input("project name")
def get_project_path(self) -> str:
"""Get project path from user"""
return self._get_input("project path")
def get_venv_name(self) -> str:
"""Get virtual environment name from user"""
return self._get_input("virtual environment name")| def test_find_python_installations(self): | ||
| """Test finding Python installations""" | ||
| installations = self.venv_creator.find_python_installations() | ||
| self.assertIsInstance(installations, list) | ||
| if installations: | ||
| self.assertTrue(all(os.path.exists(path) for path in installations)) |
There was a problem hiding this comment.
issue (testing): Python installations test needs mocking
Instead of relying on actual system Python installations, mock the filesystem and test specific scenarios: 1) No Python installations, 2) Multiple versions, 3) Invalid Python executables, 4) Different installation paths. This will make the test more reliable and comprehensive.
| print(f"Error details: {e.stderr}") | ||
| return False | ||
|
|
||
| def install_requirements(self, venv_path: str, requirements_path: Optional[str] = None) -> Optional[bool]: |
There was a problem hiding this comment.
issue (complexity): Consider extracting requirements preprocessing logic into a separate method to reduce complexity.
The install_requirements() method has excessive nesting that can be simplified by extracting the requirements preprocessing logic. Here's how to reduce complexity while maintaining functionality:
def _preprocess_requirements(self, requirements_path: str, project_root: str) -> Tuple[str, bool]:
"""Preprocess requirements file and return temp file path and success status"""
temp_requirements = os.path.join(project_root, 'temp_requirements.txt')
missing_files = []
with open(requirements_path, mode='r', encoding='utf-8') as original:
with open(temp_requirements, mode='w', encoding='utf-8') as temp:
for line in original:
line = line.strip()
if not line.startswith('dependencies/'):
temp.write(f"{line}\n")
continue
absolute_path = os.path.abspath(os.path.join(project_root, line))
if not os.path.exists(absolute_path) or os.path.getsize(absolute_path) == 0:
missing_files.append(line)
print(f"Warning: Invalid dependency file: {line}")
else:
temp.write(f"{absolute_path}\n")
if missing_files:
print("\nMissing or invalid dependency files:")
for file in missing_files:
print(f"- {file}")
return temp_requirements, False
return temp_requirements, True
def install_requirements(self, venv_path: str, requirements_path: Optional[str] = None) -> Optional[bool]:
"""Install requirements if requirements.txt exists"""
if not requirements_path or not os.path.exists(requirements_path):
return None
project_root = os.path.dirname(requirements_path)
temp_requirements, preprocessing_success = self._preprocess_requirements(requirements_path, project_root)
try:
if not preprocessing_success:
return False
pip_path = self._get_pip_path(venv_path)
print("\nInstalling requirements...")
result = subprocess.run(
[pip_path, 'install', '-r', temp_requirements],
check=True, capture_output=True, text=True
)
if result.stderr:
print(f"Warnings during installation: {result.stderr}")
return True
except subprocess.CalledProcessError as e:
print(f"Error installing requirements: {e}")
if e.stderr:
print(f"Error details: {e.stderr}")
return False
except Exception as e:
print(f"Unexpected error during requirements installation: {e}")
return False
finally:
if os.path.exists(temp_requirements):
os.remove(temp_requirements)This refactoring:
- Separates requirements preprocessing into a dedicated method
- Reduces nesting depth by using early returns and continue statements
- Maintains all error handling and functionality
- Improves readability by grouping related operations
| 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 'venv_creator.py' for managing Python virtual environments and update the CI workflow to include enhanced testing, linting, and security checks. Introduce a new test suite for the 'VenvCreator' class to ensure robust functionality.
New Features:
Enhancements:
CI:
Tests: