Prepare project config for Text2Gremlin - #76
Conversation
|
@codecov-ai-reviewer review |
Walkthrough本次变更为 Changes项目配置与检查规则更新
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces configuration changes and dependency specifications for a new text2gremlin component, including updates to .licenserc.yaml, Ruff linting exclusions in pyproject.toml, and a new requirements.txt file. The review feedback highlights three key areas for improvement: first, it warns that suppressing F811 (redefined names) in GremlinTransVisitor.py may mask bugs caused by Python's lack of method overloading; second, it recommends renaming the new directories and files to follow PEP 8 snake_case conventions rather than suppressing N999 lint errors; and third, it suggests integrating the new dependencies into the root pyproject.toml to maintain unified dependency management via uv instead of using a standalone requirements.txt.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| "text2gremlin/AST_Text2Gremlin/base/GremlinTransVisitor.py" = [ | ||
| "F403", # ANTLR visitor compatibility import style. | ||
| "F811", # Visitor methods mirror grammar alternatives and may intentionally repeat names. | ||
| "T20", # Parser diagnostics are printed by the standalone visitor helper. | ||
| ] |
There was a problem hiding this comment.
In Python, defining multiple methods with the same name within a class will silently overwrite the previous definitions (as Python does not support method overloading). If the upcoming GremlinTransVisitor.py indeed repeats method names, only the last one defined will be executed, which is likely a bug.\n\nANTLR4 typically generates unique visitor method names for each labeled alternative (e.g., visitAdd, visitSub). Please verify if F811 is truly needed, or if the visitor implementation needs to be corrected to avoid duplicate method definitions.
There was a problem hiding this comment.
Not changing this in PR1. This PR only prepares project plumbing and does not include GremlinTransVisitor.py. I verified the concern against the full Text2Gremlin branch; duplicate visitor method names need to be handled or justified in the later core/quality PR where the visitor implementation is actually introduced.
| "text2gremlin/AST_Text2Gremlin/base/*.py" = [ | ||
| "N999", # Keep existing standalone module names such as GremlinBase.py. | ||
| ] |
There was a problem hiding this comment.
PEP 8 recommends using short, all-lowercase names for modules and packages, with underscores only if they improve readability (e.g., ast_text2gremlin, gremlin_base.py, gremlin_trans_visitor.py, traversal_generator.py).\n\nSince this is a brand new feature being introduced to the repository, it is highly recommended to rename these directories and files to follow standard snake_case naming conventions. This will improve maintainability, align with the rest of the codebase, and eliminate the need for suppressing N999 (invalid module name) lint errors.
References
- PEP 8 recommends using short, all-lowercase names for modules and packages, with underscores only if they improve readability. (link)
There was a problem hiding this comment.
Not changing this in PR1. Renaming AST_Text2Gremlin and the existing visitor/generator modules would affect imports, docs, and follow-up split PR boundaries. This series is intentionally splitting the existing implementation for review without doing a broad rename/refactor; naming cleanup can be handled as a separate follow-up if maintainers want it.
| antlr4-python3-runtime==4.13.1 | ||
| openai>=1.30.0 | ||
| pandas>=2.0.0 | ||
| pydantic>=2.0.0 | ||
| pytest>=8.0.0 |
There was a problem hiding this comment.
The repository uses uv for workspace and dependency management, with centralized constraints defined in pyproject.toml (under tool.uv.constraint-dependencies). Introducing a standalone requirements.txt in a subdirectory bypasses this unified dependency management and can lead to version drift or conflicts (e.g., installing different versions of pandas, pydantic, or openai than those constrained in the root).\n\nIt is highly recommended to integrate text2gremlin's dependencies into the root pyproject.toml (for example, by adding a text2gremlin extra under [project.optional-dependencies] or managing it as a workspace member) to ensure consistent dependency resolution, locking, and ease of installation across environments.
There was a problem hiding this comment.
Fixed in 24b25b8. requirements.txt now matches the root workspace constraints: openai~=1.61.0, pandas~=2.2.3, pydantic~=2.10.6, and pytest~=8.0.0, while keeping antlr4-python3-runtime==4.13.1 for the generated parser runtime.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
text2gremlin/AST_Text2Gremlin/requirements.txt (1)
1-6: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win子项目依赖版本与根级约束不一致
requirements.txt中的版本约束比pyproject.toml的constraint-dependencies宽松：
openai>=1.30.0对比openai~=1.61.0pandas>=2.0.0对比pandas~=2.2.3pydantic>=2.0.0对比pydantic~=2.10.6若该
requirements.txt被独立使用（如直接pip install -r），可能安装与 workspace 不兼容的版本。建议与根级约束对齐，或添加注释说明其使用场景。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@text2gremlin/AST_Text2Gremlin/requirements.txt` around lines 1 - 6, Align the version constraints in requirements.txt with the workspace constraints defined in pyproject.toml for openai, pandas, and pydantic so the subproject does not install incompatible versions when used standalone. Update the dependency pins in the requirements file to match the root-level constraint-dependencies, or add a clear note in the file explaining that these looser bounds are intentional and only for a specific install scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@text2gremlin/AST_Text2Gremlin/requirements.txt`:
- Around line 1-6: Align the version constraints in requirements.txt with the
workspace constraints defined in pyproject.toml for openai, pandas, and pydantic
so the subproject does not install incompatible versions when used standalone.
Update the dependency pins in the requirements file to match the root-level
constraint-dependencies, or add a clear note in the file explaining that these
looser bounds are intentional and only for a specific install scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44a2b8e4-b48d-4ef2-a071-acf4853b35bb
📒 Files selected for processing (4)
.licenserc.yamlpyproject.tomltext2gremlin/AST_Text2Gremlin/.gitignoretext2gremlin/AST_Text2Gremlin/requirements.txt
There was a problem hiding this comment.
Pull request overview
This PR prepares project-level plumbing for upcoming Text2Gremlin work by adding a small Text2Gremlin subdirectory scaffold and tightening repository-wide tooling configuration to accommodate generated/parser-related files.
Changes:
- Added a Text2Gremlin-side
requirements.txtand local.gitignorefor thetext2gremlin/AST_Text2Gremlin/area. - Updated Ruff configuration to exclude generated parser code and apply targeted per-file ignores for upcoming Text2Gremlin scripts.
- Updated license-header tooling exclusions to ignore ANTLR metadata files and CSV artifacts.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| text2gremlin/AST_Text2Gremlin/requirements.txt | Introduces dependency list for the Text2Gremlin AST subproject. |
| text2gremlin/AST_Text2Gremlin/.gitignore | Adds local ignore rules for config/output artifacts within the Text2Gremlin subtree. |
| pyproject.toml | Extends Ruff excludes and per-file ignores to accommodate generated/parser and script-style files. |
| .licenserc.yaml | Excludes ANTLR metadata and CSV files from license-header checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| antlr4-python3-runtime==4.13.1 | ||
| openai>=1.30.0 | ||
| pandas>=2.0.0 | ||
| pydantic>=2.0.0 | ||
| pytest>=8.0.0 |
There was a problem hiding this comment.
Fixed in 24b25b8. The standalone Text2Gremlin requirements now align with the root workspace constraints for OpenAI, pandas, pydantic, and pytest, so standalone installs do not drift from the workspace dependency set.
| # 配置文件 | ||
| config.json | ||
| .env | ||
| output/ | ||
|
|
There was a problem hiding this comment.
Not changing this in PR1. The ANTLR parser artifacts are intentionally committed in the follow-up parser PR so reviewers can inspect the generated parser alongside the grammar. .licenserc.yaml excludes .interp, .tokens, and .csv from license-header checks, but .gitignore should not hide the parser artifacts we plan to submit.
|
Review follow-up summary: Accepted and fixed:
Not changed in this PR:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
text2gremlin/AST_Text2Gremlin/requirements.txt (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win请确认
pytest是否应作为运行时依赖。如果这个
requirements.txt会被部署环境直接安装，pytest会把测试工具一并带入生产环境；更合适的是拆到单独的 dev/test 依赖文件，或者确认该文件仅用于本地开发/CI。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@text2gremlin/AST_Text2Gremlin/requirements.txt` at line 5, `pytest` appears to be listed in the main runtime requirements, which would install test tooling in deployment environments. Move `pytest` out of this requirements file into a dedicated dev/test dependencies file, or clearly scope this file to development/CI only; keep the dependency list in `requirements.txt` limited to runtime packages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@text2gremlin/AST_Text2Gremlin/requirements.txt`:
- Line 5: `pytest` appears to be listed in the main runtime requirements, which
would install test tooling in deployment environments. Move `pytest` out of this
requirements file into a dedicated dev/test dependencies file, or clearly scope
this file to development/CI only; keep the dependency list in `requirements.txt`
limited to runtime packages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b682c5dd-2455-46e5-b71a-28cc589d36a7
📒 Files selected for processing (1)
text2gremlin/AST_Text2Gremlin/requirements.txt
This is the first split-out PR from the larger Text2Gremlin work in #52.
The full #52 PR is intentionally kept open for context while the work is submitted in smaller, reviewable parts. The original PR was too large to review safely as a single change, so this series will separate project plumbing, parser artifacts, AST generalization, demo data/templates, CLI/docs, LLM augmentation, DPO generation, and follow-up quality fixes.
This PR contains only the project plumbing needed before the implementation PRs:
It intentionally does not include parser code, AST/generalization logic, movie demo data, CLI/docs, LLM pipeline code, or DPO generation. Those will be submitted as follow-up PRs in smaller layers.
Validation:
uv run pythonTOML parse check forpyproject.tomluv run ruff format --check .uv run ruff check .Summary by CodeRabbit