Fix verbose logging and remove unused variables/imports for CI #55
Conversation
📝 WalkthroughWalkthroughA submodule reference for databus-python-client is updated to a new commit. Additionally, an import alias for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
databus-python-client (4)
1-6: Major inconsistency between PR objectives and submodule commit content.The PR title states "Fix verbose logging and remove unused variables/imports for CI" and the description mentions resolving ruff linting issues and introducing a verbose parameter. However, this submodule commit (189458e) is titled "cli: add mkdist validations, completion helper, tests and docs" and introduces entirely new CLI functionality (mkdist and completion commands).
This appears to be the wrong submodule commit reference for this PR's stated objectives. Please verify that the correct commit is being referenced.
65-105: Remove duplicate documentation sections.Lines 86-105 are an exact duplicate of lines 65-85, repeating the mkdist and completion command documentation. This duplication should be removed to maintain clean documentation.
📝 Proposed fix
Remove lines 86-105 entirely as they duplicate the content already present in lines 65-85.
159-163: Remove redundant import of json module.The
jsonmodule is already imported at line 114, but line 160 imports it again as_json. This is unnecessary.♻️ Proposed fix
dist = client.create_distribution(url=url, cvs=sorted_cvs, file_format=file_format, compression=compression, sha256_length_tuple=sha_tuple) if json_output: - import json as _json - click.echo(_json.dumps({"distribution": dist})) + click.echo(json.dumps({"distribution": dist})) else: click.echo(dist)
24-60: Do not rename--version-idto--versionid—it violates CLI conventions and breaks backward compatibility.The proposed change to rename the parameter from kebab-case (
--version-id) to a single word (--versionid) breaks the standard CLI naming convention and will break existing scripts and workflows. If this rename is intentional, it must be clearly documented as a breaking change and justified. Otherwise, keep the parameter as--version-idto maintain backward compatibility and follow established CLI conventions.
🧹 Nitpick comments (1)
databus-python-client (1)
179-221: Consider adding tests for additional validation scenarios.The current tests cover the happy path and basic error cases, but several validation branches remain untested:
- Duplicate CV key detection (cli.py line 143-145)
- Forbidden characters in CV keys like
|or_(cli.py line 141-142)- JSON output option (
--json-outputflag, cli.py line 132)- Edge cases with missing optional parameters
Adding these tests would improve coverage and prevent regressions in the validation logic.
📋 Example tests to add
def test_mkdist_duplicate_cv_key(): runner = CliRunner() res = runner.invoke(cli.app, [ 'mkdist', 'https://example.org/file', '--cv', 'lang=en', '--cv', 'lang=fr' # duplicate key ]) assert res.exit_code != 0 assert 'Duplicate content-variant key' in res.output def test_mkdist_forbidden_characters_in_cv_key(): runner = CliRunner() res = runner.invoke(cli.app, [ 'mkdist', 'https://example.org/file', '--cv', 'la|ng=en' # forbidden | character ]) assert res.exit_code != 0 assert 'Invalid characters' in res.output def test_mkdist_json_output(): runner = CliRunner() res = runner.invoke(cli.app, [ 'mkdist', 'https://example.org/file', '--cv', 'lang=en', '--json-output' ]) assert res.exit_code == 0 import json data = json.loads(res.output) assert 'distribution' in data
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_download.py`:
- Around line 2-5: Remove the Git merge conflict markers (<<<<<<< HEAD, =======,
>>>>>>> main) from tests/test_download.py and resolve the conflict by keeping
the intended import or deleting it; specifically decide whether the import
statement "import databusclient.client as cl" is required by the tests—if not,
remove that unused import; ensure the file has no conflict markers and imports
only what tests use so test collection and linting succeed.
| <<<<<<< HEAD | ||
| import databusclient.client as cl | ||
| ======= | ||
| >>>>>>> main |
There was a problem hiding this comment.
Resolve merge conflict markers; file is currently invalid.
The <<<<<<<, =======, >>>>>>> markers will break test collection and linting. Resolve the conflict and remove the unused import unless it’s actually needed.
🧩 Proposed fix
-<<<<<<< HEAD
-import databusclient.client as cl
-=======
->>>>>>> main📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <<<<<<< HEAD | |
| import databusclient.client as cl | |
| ======= | |
| >>>>>>> main |
🤖 Prompt for AI Agents
In `@tests/test_download.py` around lines 2 - 5, Remove the Git merge conflict
markers (<<<<<<< HEAD, =======, >>>>>>> main) from tests/test_download.py and
resolve the conflict by keeping the intended import or deleting it; specifically
decide whether the import statement "import databusclient.client as cl" is
required by the tests—if not, remove that unused import; ensure the file has no
conflict markers and imports only what tests use so test collection and linting
succeed.
|
It looks like something has gone wrong. The changes that have been made appear quite strange.
|
2 participants
Pull Request
Description
This PR resolves linting issues reported by
ruffand introduces an optionalverboseparameter to control download log output.What this PR does
ruffverboseparameter to the download logic to optionally print progress logsWhy
Related Issues
Fixes #27 (supersedes earlier PR with CI failures)
Type of change
Checklist
python -m ruff check .— all checks passedSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.