feat(INT-814): BDK template updates - #30
Conversation
WalkthroughAdds cspell configuration with custom dictionary, introduces a new run_test CLI in poetry_scripts to run targeted pytest commands with optional snapshot/cassette flags, and updates pyproject to remove a plugin, add syrupy, and register new scripts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Poetry as poetry (scripts)
participant PS as poetry_scripts.run_test
participant PyTest as pytest
Dev->>Poetry: poetry run test -k "<expr>" [-u] [-r]
Poetry->>PS: invoke run_test()
PS->>PS: parse args (-k, -u, -r)
PS->>PyTest: exec pytest -k "<expr>" [--snapshot-update] [--record-mode=rewrite]
PyTest-->>PS: exit code
PS-->>Poetry: propagate exit code
Poetry-->>Dev: exit with status
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.12.2){{cookiecutter.project_folder_name}}/poetry_scripts.py�[1;31mruff failed�[0m Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
{{cookiecutter.project_folder_name}}/cspell.json (1)
1-7: Optional hardening: add common ignores and dictionaries to reduce false positives.Not required for this PR’s goal, but adding standard ignores and enabling the Python/software terms dictionaries will make cspell quieter out of the box.
Example enhancement:
{ "$schema": "https://raw.githubusercontent.com/streetsidesoftware/cspell/main/cspell.schema.json", "version": "0.2", + "language": "en", + "useGitignore": true, + "ignorePaths": [ + ".git/**", + ".venv/**", + "node_modules/**", + "dist/**", + "build/**", + "**/__pycache__/**" + ], + "dictionaries": ["python", "softwareTerms"], "words": [ "kognitos" ] }{{cookiecutter.project_folder_name}}/poetry_scripts.py (2)
21-26: Make -k required via argparse instead of manual check.Leverage argparse’s validation rather than a follow-up manual guard.
- parser.add_argument( - "-k", - metavar="<expression>", - help="(required) run tests matching the given expression." - ) + parser.add_argument( + "-k", + metavar="<expression>", + required=True, + help="run tests matching the given expression." + )
12-15: Nit: keep pytest invocation style consistent.run_tests uses “poetry run pytest …” while run_test uses “python -m pytest …”. Both work (since the console script runs in the venv), but consistency helps debugging.
If you want them aligned, either switch run_tests to the argv style:
- return run_cmd("poetry run pytest -vv --junit-xml=test-results.xml") + rc = subprocess.run( + [sys.executable, "-m", "pytest", "-vv", "--junit-xml=test-results.xml"], + env=os.environ.copy() + ).returncode + return rcOr flip run_test to “poetry run …” (with safe quoting), but the argv approach above is safer cross-platform.
{{cookiecutter.project_folder_name}}/pyproject.toml (1)
36-36: Reconcile doc script with removed Poetry plugin.doc = "poetry_scripts:run_doc" is still present, but the underlying command currently depends on kognitos-bdk-poetry-plugin. Without the plugin, “poetry bdk usage” will fail on new projects.
Pick one:
- Remove the doc script until there’s a non-plugin flow, or
- Keep it but update run_doc to emit a clear message when the plugin is missing (see the suggested diff in poetry_scripts.py), or
- Switch to an alternative doc generation command that doesn’t require the Poetry plugin.
If you want to remove the script now:
[tool.poetry.scripts] test="poetry_scripts:run_test" tests = "poetry_scripts:run_tests" format = "poetry_scripts:run_format" lint = "poetry_scripts:run_lint" type-check = "poetry_scripts:run_type_check" -doc = "poetry_scripts:run_doc" host = "poetry_scripts:run_host"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
{{cookiecutter.project_folder_name}}/cspell.json(1 hunks){{cookiecutter.project_folder_name}}/poetry_scripts.py(1 hunks){{cookiecutter.project_folder_name}}/pyproject.toml(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
{{cookiecutter.project_folder_name}}/cspell.json
[error] 6-6: Expected an array, an object, or a literal but instead found ']'.
Expected an array, an object, or a literal here.
(parse)
[error] 6-7: Expected a property but instead found '}'.
Expected a property here.
(parse)
🔇 Additional comments (2)
{{cookiecutter.project_folder_name}}/pyproject.toml (2)
28-28: Good add: syrupy enables the --snapshot-update flag.This aligns with run_test’s -u/--update behavior and will be auto-discovered by pytest.
31-31: New test entry point looks good.Mapping test="poetry_scripts:run_test" provides a convenient single-test runner. Works well with the argparse changes suggested above.
| "words": [ | ||
| "kognitos", | ||
| ], | ||
| } |
There was a problem hiding this comment.
Fix JSON: trailing comma breaks parsing (Biome parse errors).
The trailing comma after "kognitos" makes the JSON invalid and matches the Biome errors reported. Remove it to fix parsing.
Apply this diff:
- "words": [
- "kognitos",
- ],
+ "words": [
+ "kognitos"
+ ]📝 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.
| "words": [ | |
| "kognitos", | |
| ], | |
| } | |
| "words": [ | |
| "kognitos" | |
| ] | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 6-6: Expected an array, an object, or a literal but instead found ']'.
Expected an array, an object, or a literal here.
(parse)
[error] 6-7: Expected a property but instead found '}'.
Expected a property here.
(parse)
🤖 Prompt for AI Agents
In {{cookiecutter.project_folder_name}}/cspell.json around lines 4 to 7, the
JSON contains a trailing comma after the last item "kognitos" which makes the
file invalid and causes Biome parse errors; remove the trailing comma so the
"words" array is properly closed and the JSON object ends with a valid closing
brace.
| # Require -k | ||
| if not args.k: | ||
| parser.print_help() | ||
| sys.exit(1) | ||
|
|
||
| # Build pytest command string | ||
| cmd = f"{sys.executable} -m pytest -k '{args.k}'" | ||
|
|
||
| if args.update: | ||
| cmd += " --snapshot-update" | ||
|
|
||
| if args.record: | ||
| cmd += " --record-mode=rewrite" | ||
|
|
||
| sys.exit(run_cmd(cmd)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix shell injection risk and Windows quoting in run_test.
Building a shell string with user-provided -k and running it with shell=True is both unsafe and breaks on Windows (single quotes are not parsed by cmd.exe). Build an argv list and avoid the shell.
- # Require -k
- if not args.k:
- parser.print_help()
- sys.exit(1)
-
- # Build pytest command string
- cmd = f"{sys.executable} -m pytest -k '{args.k}'"
-
- if args.update:
- cmd += " --snapshot-update"
-
- if args.record:
- cmd += " --record-mode=rewrite"
-
- sys.exit(run_cmd(cmd))
+ # Build pytest argv and avoid shell
+ cmd = [sys.executable, "-m", "pytest", "-k", args.k]
+
+ if args.update:
+ cmd.append("--snapshot-update")
+
+ if args.record:
+ cmd.append("--record-mode=rewrite")
+
+ rc = subprocess.run(cmd, env=os.environ.copy()).returncode
+ sys.exit(rc)Committable suggestion skipped: line range outside the PR's diff.
| def run_doc(): | ||
| # generate documentation | ||
| return run_cmd("poetry bdk usage") | ||
|
|
There was a problem hiding this comment.
doc command likely broken after removing the poetry plugin.
This still calls “poetry bdk usage”, which is provided by kognitos-bdk-poetry-plugin. Since the template no longer includes that plugin, this will fail on fresh projects.
Two options; pick one:
- Option A (graceful message): detect failure and print an actionable hint.
def run_doc():
- # generate documentation
- return run_cmd("poetry bdk usage")
+ # generate documentation (requires kognitos-bdk-poetry-plugin)
+ rc = run_cmd("poetry bdk usage")
+ if rc != 0:
+ print(
+ "poetry bdk usage failed. The 'kognitos-bdk-poetry-plugin' is not installed.\n"
+ "Install it globally with: poetry self add kognitos-bdk-poetry-plugin\n"
+ "Or remove/override the 'doc' script in pyproject.toml.",
+ file=sys.stderr,
+ )
+ return rc- Option B (remove coupling): drop the “doc” script from pyproject.toml and/or replace this function with whichever non-plugin doc flow you prefer.
Committable suggestion skipped: line range outside the PR's diff.
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
Summary by CodeRabbit
Chores
Tests