Standardize on venv/ for Python virtual environments - #75
Conversation
Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
Add .venv/** to Codacy exclude_paths
Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
Remove .venv support and standardize on venv/ naming
Updated issue creation scripts and documentation Added .gitattributes for consistent line endings across platforms Updated .venv with new dependencies and development tools Updated code review tracking documentation with new issues and processes
There was a problem hiding this comment.
Pull request overview
This PR standardizes Python virtual environment naming across the repo by documenting/enforcing venv/ usage (instead of .venv/), updates issue-creation documentation/scripts accordingly, and adds a PowerShell wrapper to make issue creation easier on Windows.
Changes:
- Remove
.venv/from.gitignoreand update setup docs to explicitly usepython -m venv venv. - Update issue-creation docs/scripts to reflect variable issue counts and add
run-issue-creation.ps1. - Add
.gitattributesrule to enforce LF endings for.shscripts.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/run-issue-creation.sh | Updates messaging to reflect variable issue counts. |
| scripts/run-issue-creation.ps1 | Adds PowerShell wrapper for running the Python issue creation script. |
| scripts/README.md | Updates script descriptions and comparison table; documents the new PS wrapper. |
| docs/TEST_FRAMEWORK.md | Clarifies venv should be used (not .venv). |
| docs/ISSUE_GENERATION_PROCESS.md | Adds PS wrapper method + WSL notes. |
| docs/CODE_REVIEW_TEST_RESULTS.md | Clarifies “15 issues” refers to the 2026-02-02 review. |
| docs/CI_CD.md | Documents .venv removal/venv standardization and Codacy exclude list. |
| backend/requirements.txt | Minor comment/formatting adjustments in dependency list. |
| _bmad-output/implementation-artifacts/code-review-issues-tracking.md | Adds a tracking artifact for created issues. |
| VENV_SETUP.md | Adds explicit venv/ naming convention note. |
| README.md | Adds explicit guidance to use venv/ (not .venv/). |
| ISSUE_CREATION_INSTRUCTIONS.md | Updates wording to reflect variable issue counts/log contents. |
| CONTRIBUTING.md | Adds explicit venv/ setup steps. |
| .gitignore | Removes .venv/ ignore entry. |
| .gitattributes | Enforces LF line endings for .sh scripts. |
| $env:GITHUB_REPOSITORY = $repo | ||
|
|
||
| $pythonCmd = Get-Command py -ErrorAction SilentlyContinue | ||
| if ($null -ne $pythonCmd) { | ||
| & py -3 "$scriptDir\create-github-issues.py" |
There was a problem hiding this comment.
$repo / $env:GITHUB_REPOSITORY is set here, but create-github-issues.py uses a hard-coded REPO = "tim-dickey/trivia-app" and does not read the env var. This makes the wrapper’s repository selection misleading (and reduces reusability for forks). Either remove the repo/env wiring from the wrapper or update the Python script to read from the environment with a sane default.
| ### P0 (unknown) | ||
|
|
||
| - [ ] #61 - [P0] Consolidate CI/CD Workflows to Eliminate Duplicate Test Runs | ||
| - [ ] #62 - [P0] Implement Organization Scoping Middleware for Multi-Tenancy | ||
| - [ ] #63 - [P0] Fix Test Database Configuration (PostgreSQL in CI) | ||
| - [ ] #64 - [P0] Document Required GitHub Secrets for CI/CD | ||
|
|
||
| ### P1 (unknown) |
There was a problem hiding this comment.
The priority name is shown as (unknown) here, which makes the tracking output ambiguous. If this file is meant to be a durable artifact, update generation so P0/P1 map to meaningful names (e.g., Critical/High) rather than unknown.
| ### P0 (unknown) | |
| - [ ] #61 - [P0] Consolidate CI/CD Workflows to Eliminate Duplicate Test Runs | |
| - [ ] #62 - [P0] Implement Organization Scoping Middleware for Multi-Tenancy | |
| - [ ] #63 - [P0] Fix Test Database Configuration (PostgreSQL in CI) | |
| - [ ] #64 - [P0] Document Required GitHub Secrets for CI/CD | |
| ### P1 (unknown) | |
| ### P0 (Critical) | |
| - [ ] #61 - [P0] Consolidate CI/CD Workflows to Eliminate Duplicate Test Runs | |
| - [ ] #62 - [P0] Implement Organization Scoping Middleware for Multi-Tenancy | |
| - [ ] #63 - [P0] Fix Test Database Configuration (PostgreSQL in CI) | |
| - [ ] #64 - [P0] Document Required GitHub Secrets for CI/CD | |
| ### P1 (High) |
| - [ ] #61 - [P0] Consolidate CI/CD Workflows to Eliminate Duplicate Test Runs | ||
| - [ ] #62 - [P0] Implement Organization Scoping Middleware for Multi-Tenancy | ||
| - [ ] #63 - [P0] Fix Test Database Configuration (PostgreSQL in CI) | ||
| - [ ] #64 - [P0] Document Required GitHub Secrets for CI/CD |
There was a problem hiding this comment.
The P0 list appears incomplete for the referenced source review: code-review-issues-p0.json defines 5 P0 issues (including P0-3: WebSocket infrastructure), but only 4 are tracked here. If this file is generated, consider regenerating after creation (or annotate that some issues were intentionally skipped).
| - [ ] #64 - [P0] Document Required GitHub Secrets for CI/CD | |
| - [ ] #64 - [P0] Document Required GitHub Secrets for CI/CD | |
| - [ ] (TBD) - [P0] P0-3: WebSocket infrastructure (issue to be created and linked) |
| pydantic-settings==2.1.0 | ||
| email-validator==2.1.0 |
There was a problem hiding this comment.
There are trailing spaces after the pinned versions here. While pip will usually tolerate it, these tend to create noisy diffs and can trip some tooling; please remove the trailing whitespace.
| pydantic-settings==2.1.0 | |
| email-validator==2.1.0 | |
| pydantic-settings==2.1.0 | |
| email-validator==2.1.0 |
script to fix the unicode encoding issue.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@tim-dickey I've opened a new pull request, #79, to work on those changes. Once the pull request is ready, I'll request review from you. |
Codacy's Analysis Summary0 new issue (≤ 0 issue) Review Pull Request in Codacy →
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Make create-github-issues.py read GITHUB_REPOSITORY env var with fallback - Update tracking markdown with proper priority names (Critical, High) - Add missing P0-3 WebSocket infrastructure issue to tracking - Remove trailing whitespace from requirements.txt Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
Replace TBD placeholder with explanatory note about P0-3 not being created, avoiding confusion in the checklist tracking Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
Normalize env var value with .strip() and fall back to default when empty or whitespace-only, preventing gh issue create failures with empty --repo flag Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
Address PR #75 review feedback: env var support, priority names, and whitespace
Co-authored-by: codacy-production[bot] <61871480+codacy-production[bot]@users.noreply.github.com>
Co-authored-by: codacy-production[bot] <61871480+codacy-production[bot]@users.noreply.github.com>
|
@copilot - Resolve this conflict |
|
@tim-dickey I've opened a new pull request, #80, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…s to 1.13.5 Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
Co-authored-by: codacy-production[bot] <61871480+codacy-production[bot]@users.noreply.github.com>
Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
The pull request successfully standardizes the project on the venv/ directory for Python virtual environments. This change is reflected across documentation, CI/CD configurations, and issue-creation scripts. The exclusion of both venv/ and .venv/ in Codacy paths is a proactive measure for security and performance.
💡 Codacy uses AI. Check for mistakes.
| # If gh is not authenticated, it will use the current session's credentials | ||
|
|
||
| echo "Note: Creating 20 issues (5 P0, 5 P1, 5 P2, 5 P3)" | ||
| echo "Note: Issue count varies based on BMAD review results" |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: While updating this script to generalize the issue count, consider making the REPO variable flexible to align with the environment variable support you added to the underlying Python script.
Try running the following prompt in your IDE agent:
Update scripts/run-issue-creation.sh to set the REPO variable using the GITHUB_REPOSITORY environment variable if it exists, otherwise defaulting to 'tim-dickey/trivia-app'. Use the shell syntax REPO="${GITHUB_REPOSITORY:-tim-dickey/trivia-app}".
| Write-Host '' | ||
| Write-Host 'Please wait while issues are created...' | ||
|
|
||
| $scriptDir = Split-Path -Parent $MyInvocation.MyCommand.Path |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: You can simplify this by using the built-in $PSScriptRoot variable.
This might be a simple fix:
| $scriptDir = Split-Path -Parent $MyInvocation.MyCommand.Path | |
| $scriptDir = $PSScriptRoot |
…fix Docker config Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
Resolve merge conflicts, patch critical security vulnerabilities, and improve configuration
|
@copilot - Review and comment on these conflicts |
|
@tim-dickey I've opened a new pull request, #81, to work on those changes. Once the pull request is ready, I'll request review from you. |
Comprehensive merge conflict analysis for venv standardization PR
|
@copilot - Review these merge conflicts |
|
@tim-dickey I've opened a new pull request, #83, to work on those changes. Once the pull request is ready, I'll request review from you. |
- Use Node 22.2.0 (newer) and keep semgrep tool in Codacy config - Use cryptography 46.0.5 (newer, fixes SECT curve vulnerability) - Use environment variable extraction in docker-entrypoint.sh for flexibility - Use SECRET_KEY with default fallback in docker-compose.yml - Merge frontend dependencies without duplicates - Regenerate package-lock.json after package.json merge
…gain Brings in merge conflict resolutions from main branch.
Co-authored-by: tim-dickey <80638631+tim-dickey@users.noreply.github.com>
Resolve merge conflicts between fix/test-unicode-encoding and main
Remove support for
.venv/and enforce the use ofvenv/for consistency across the project. Update documentation and scripts to reflect this change, ensuring clarity in setup instructions and issue creation processes. Additionally, optimize Codacy analysis by excluding unnecessary paths.