chore: enforce python docstring coverage#41
Conversation
|
@coderabbitai review |
📝 WalkthroughWalkthroughservices/analysis-engine 범위에 대한 Python 도큐스트링 요구사항과 검사 스크립트가 추가·강화되고, 여러 검사 스크립트와 테스트에 도큐스트링이 삽입되었으며 릴리스 패키징의 바이너리 경로 결정 로직에 타깃 트리플 기반 오버라이드가 추가되었습니다. 테스트 기대값에 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/release/package_desktop_artifact.py (1)
61-70:⚠️ Potential issue | 🟠 Major
BANDSCOPE_TARGET_TRIPLE만 설정된 Windows 크로스 빌드가 깨집니다.여기서는 바이너리 확장자를
normalized_platform()로 결정하므로, 호스트가 Linux/macOS이고BANDSCOPE_TARGET_TRIPLE=x86_64-pc-windows-msvc처럼 타깃만 지정되면 경로를bandscope-desktop으로 계산합니다. 실제 산출물은.exe라서main()이FileNotFoundError로 끝납니다. 타깃 트리플이 있으면 플랫폼/확장자도 그 값에서 파생시키거나, 최소한BANDSCOPE_ARTIFACT_OS가 함께 없을 때 명시적으로 실패시키는 편이 안전합니다.🔧 제안 수정안
def expected_binary_path(repo_root: Path) -> Path: """Return the expected desktop binary path for the selected target triple.""" - system = normalized_platform() + target_triple = os.environ.get("BANDSCOPE_TARGET_TRIPLE") + if target_triple and "windows" in target_triple: + system = "windows" + elif target_triple and "apple-darwin" in target_triple: + system = "macos" + else: + system = normalized_platform() binary_name = ( "bandscope-desktop.exe" if system == "windows" else "bandscope-desktop" ) target_root = repo_root / "apps" / "desktop" / "src-tauri" / "target" - if target_triple := os.environ.get("BANDSCOPE_TARGET_TRIPLE"): + if target_triple: target_root = target_root / target_triple return target_root / "release" / binary_name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/package_desktop_artifact.py` around lines 61 - 70, expected_binary_path currently picks the binary extension from normalized_platform(), so when only BANDSCOPE_TARGET_TRIPLE is set (e.g., x86_64-pc-windows-msvc) on a non-Windows host you compute "bandscope-desktop" and later main() raises FileNotFoundError; change expected_binary_path to derive the OS/extension from BANDSCOPE_TARGET_TRIPLE when present (detect "windows" in the triple and use ".exe"), or if you prefer require an explicit override use BANDSCOPE_ARTIFACT_OS and fail fast with a clear error if neither BANDSCOPE_ARTIFACT_OS nor a recognizable BANDSCOPE_TARGET_TRIPLE is provided; update logic around normalized_platform(), BANDSCOPE_TARGET_TRIPLE and BANDSCOPE_ARTIFACT_OS in expected_binary_path accordingly so the computed path matches the actual artifact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/release/package_desktop_artifact.py`:
- Around line 61-70: expected_binary_path currently picks the binary extension
from normalized_platform(), so when only BANDSCOPE_TARGET_TRIPLE is set (e.g.,
x86_64-pc-windows-msvc) on a non-Windows host you compute "bandscope-desktop"
and later main() raises FileNotFoundError; change expected_binary_path to derive
the OS/extension from BANDSCOPE_TARGET_TRIPLE when present (detect "windows" in
the triple and use ".exe"), or if you prefer require an explicit override use
BANDSCOPE_ARTIFACT_OS and fail fast with a clear error if neither
BANDSCOPE_ARTIFACT_OS nor a recognizable BANDSCOPE_TARGET_TRIPLE is provided;
update logic around normalized_platform(), BANDSCOPE_TARGET_TRIPLE and
BANDSCOPE_ARTIFACT_OS in expected_binary_path accordingly so the computed path
matches the actual artifact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: feff9db4-b1c0-4375-b882-01da316ce6d2
📒 Files selected for processing (14)
ARCHITECTURE.mdpackage.jsonscripts/checks/security_gates.pyscripts/checks/verify_docs.pyscripts/checks/verify_github_bootstrap_policy.pyscripts/checks/verify_security_notes.pyscripts/checks/verify_supply_chain.pyscripts/release/package_desktop_artifact.pyservices/analysis-engine/src/bandscope_analysis/health.pyservices/analysis-engine/tests/conftest.pyservices/analysis-engine/tests/test_api.pyservices/analysis-engine/tests/test_health.pyservices/analysis-engine/tests/test_release_packaging.pyservices/analysis-engine/tests/test_supply_chain_policy.py
| "check:python-docstrings": "sh -c 'cd services/analysis-engine && uv run ruff check src tests ../../scripts --select D100,D101,D102,D103,D104,D105,D106,D107'", | ||
| "ruff:check": "sh -c 'cd services/analysis-engine && uv run ruff check src tests'", | ||
| "ruff:format:check": "sh -c 'cd services/analysis-engine && uv run ruff format --check src tests'", | ||
| "lint": "npm run lint:workspaces && npm run check:docs && npm run check:security-notes && npm run check:security-gates && npm run check:supply-chain && npm run check:github-bootstrap && npm run ruff:check && npm run ruff:format:check", | ||
| "lint": "npm run lint:workspaces && npm run check:docs && npm run check:security-notes && npm run check:security-gates && npm run check:supply-chain && npm run check:github-bootstrap && npm run check:python-docstrings && npm run ruff:check && npm run ruff:format:check", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What do Ruff pydocstyle rules D100-D107 cover? Do they apply only to public modules/classes/functions/methods, or also private underscore-prefixed ones?
💡 Result:
Ruff’s pydocstyle D100–D107 rules are the “missing docstring” checks:
- D100: missing docstring in public module (pydocstyle.readthedocs.io)
- D101: missing docstring in public class (pydocstyle.readthedocs.io)
- D102: missing docstring in public method (pydocstyle.readthedocs.io)
- D103: missing docstring in public function (pydocstyle.readthedocs.io)
- D104: missing docstring in public package (i.e., typically
__init__.py) (pydocstyle.readthedocs.io) - D105: missing docstring in magic method (dunder methods like
__str__) (pydocstyle.readthedocs.io) - D106: missing docstring in public nested class (pydocstyle.readthedocs.io)
- D107: missing docstring in
__init__(pydocstyle.readthedocs.io)
Do they apply to private (underscore-prefixed) definitions?
For D100–D104 and D106, they apply only to public constructs. In pydocstyle (which Ruff derives these from), a construct is public only if its parent is public and its name does not start with _ or __ (with the special case that names like __init__.py are treated as public for this purpose). Also, if __all__ is defined, only names listed there are considered public. (pydocstyle.readthedocs.io)
So, e.g. _foo() / _Bar / _baz.py won’t trigger D1xx “missing docstring” errors just because they lack docstrings. (pydocstyle.readthedocs.io)
D105 is different: it targets magic methods (dunders), not “public vs private” by underscore prefix. (pydocstyle.readthedocs.io)
Citations:
- 1: https://pydocstyle.readthedocs.io/en/latest/error_codes.html
- 2: https://pydocstyle.readthedocs.io/en/latest/error_codes.html
- 3: https://pydocstyle.readthedocs.io/en/latest/error_codes.html
- 4: https://pydocstyle.readthedocs.io/en/latest/error_codes.html
- 5: https://pydocstyle.readthedocs.io/en/latest/error_codes.html
- 6: https://pydocstyle.readthedocs.io/en/latest/error_codes.html
- 7: https://pydocstyle.readthedocs.io/en/latest/error_codes.html
- 8: https://pydocstyle.readthedocs.io/en/latest/error_codes.html
- 9: https://pydocstyle.readthedocs.io/en/latest/error_codes.html
- 10: https://pydocstyle.readthedocs.io/en/latest/error_codes.html
- 11: https://pydocstyle.readthedocs.io/en/latest/error_codes.html
D100-D107 규칙만으로는 "100% docstring coverage"를 실제로 강제하지 못합니다.
현재 선택한 Ruff 규칙은 public 심볼만을 대상으로 합니다. 따라서 _helper() 같은 private 함수나 _PrivateClass 같은 private 클래스는 docstring 검사 대상에서 제외되어 통과합니다. PR의 목표와 ARCHITECTURE.md에서 명시한 "100% docstring coverage" 정책을 일치시키려면, 정책 문구를 public 범위로 명확히 좁히거나 private 심볼까지 포함하는 추가 검사를 도입해야 합니다.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/release/package_desktop_artifact.py`:
- Around line 61-76: expected_binary_path() currently uses
BANDSCOPE_TARGET_TRIPLE to choose a binary name/path while artifact_identity()
and manifest generation still use host-derived
normalized_platform()/normalized_architecture(), causing mismatched artifact
names; update code so the target triple is parsed once into
platform/architecture values and both expected_binary_path() and
artifact_identity() consume those derived values (e.g., add a helper that parses
BANDSCOPE_TARGET_TRIPLE into target_platform and target_arch, fall back to
normalized_platform()/normalized_architecture() when unset, then use those
values inside expected_binary_path() to pick binary_name and path and in
artifact_identity() to build the artifact name) ensuring both functions
reference the same canonical target info.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: da838430-bade-43c6-a742-eb154ba72959
📒 Files selected for processing (2)
scripts/release/package_desktop_artifact.pyservices/analysis-engine/tests/test_release_packaging.py
| def expected_binary_path(repo_root: Path) -> Path: | ||
| system = normalized_platform() | ||
| """Return the expected desktop binary path for the selected target triple.""" | ||
| target_triple = os.environ.get("BANDSCOPE_TARGET_TRIPLE") | ||
| if target_triple and "windows" in target_triple: | ||
| system = "windows" | ||
| elif target_triple and "apple-darwin" in target_triple: | ||
| system = "macos" | ||
| else: | ||
| system = normalized_platform() | ||
| binary_name = ( | ||
| "bandscope-desktop.exe" if system == "windows" else "bandscope-desktop" | ||
| ) | ||
| target_root = repo_root / "apps" / "desktop" / "src-tauri" / "target" | ||
| if target_triple := os.environ.get("BANDSCOPE_TARGET_TRIPLE"): | ||
| if target_triple: | ||
| target_root = target_root / target_triple | ||
| return target_root / "release" / binary_name |
There was a problem hiding this comment.
BANDSCOPE_TARGET_TRIPLE가 패키징 결과의 단일 기준이 아닙니다.
여기서는 target triple로 바이너리 경로와 확장자를 일부 덮어쓰지만, artifact_identity()와 manifest는 여전히 호스트 기반 normalized_platform() / normalized_architecture()를 사용합니다. 그래서 Linux 호스트에서 x86_64-pc-windows-msvc만 설정하면 .exe를 패키징하면서도 산출물 이름은 bandscope-linux-amd64-...로 기록되고, 반대로 Windows 호스트에서 Linux triple을 쓰면 여기서는 여전히 .exe 경로를 기대합니다. 플랫폼/아키텍처를 target triple에서 한 번만 파생해서 expected_binary_path()와 artifact_identity()가 같이 쓰도록 맞춰야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/release/package_desktop_artifact.py` around lines 61 - 76,
expected_binary_path() currently uses BANDSCOPE_TARGET_TRIPLE to choose a binary
name/path while artifact_identity() and manifest generation still use
host-derived normalized_platform()/normalized_architecture(), causing mismatched
artifact names; update code so the target triple is parsed once into
platform/architecture values and both expected_binary_path() and
artifact_identity() consume those derived values (e.g., add a helper that parses
BANDSCOPE_TARGET_TRIPLE into target_platform and target_arch, fall back to
normalized_platform()/normalized_architecture() when unset, then use those
values inside expected_binary_path() to pick binary_name and path and in
artifact_identity() to build the artifact name) ensuring both functions
reference the same canonical target info.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Superseded by #42 to clear stale CodeRabbit review state after all checks passed. |
Pull request was closed
1 participant
Summary
Verification
npm run check:python-docstrings./scripts/harness/quickcheck.sh📝 Walkthrough
Walkthrough
저장소 전반에 걸쳐 Python docstring 요구사항과 검사 스크립트를 추가·강화하고, 일부 테스트와 릴리스 포장 로직의 경미한 조정이 포함되었습니다. 대부분은 문서화(도큐스트링) 추가이며 기능 변경은 제한적입니다.
Changes
ARCHITECTURE.md,package.jsonpackage.json에check:python-docstrings스크립트 추가 및lint스크립트에 해당 검사 삽입.scripts/checks/...scripts/checks/security_gates.py,scripts/checks/verify_docs.py,scripts/checks/verify_github_bootstrap_policy.py,scripts/checks/verify_security_notes.py,scripts/checks/verify_supply_chain.pyverify_security_notes에 보안 노트 섹션 추출/정규화 로직(소문자화 및 구분자 기반 경계 판단) 추가. 기능·시그니처는 대체로 동일.scripts/release/package_desktop_artifact.pyexpected_binary_path에 target_triple 기반 오버라이드 로직(Windows/macOS 대상 처리) 및 walrus 연산자 제거로 인한 구현 단순화 변경 포함.services/analysis-engine/src/bandscope_analysis/health.pyHealthReport에 모듈/inline docstring 추가(건강 페이로드 문서화).services/analysis-engine/tests/...conftest.py,test_api.py,test_health.py,test_release_packaging.py,test_supply_chain_policy.pytest_api.py·test_health.py의 기대 페이로드에pipeline_stages필드 (["decode","draft","separate","persist"]) 추가.test_release_packaging.py에 플랫폼/타깃 관련 새 테스트 3건 추가.Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
scripts/release/package_desktop_artifact.py와 관련 테스트(릴리스 패키징 로직 및 테스트 추가/수정)를 다룸 — 코드 수준 연관성 있음.Poem