-
-
Notifications
You must be signed in to change notification settings - Fork 169
fix filepath issue #725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix filepath issue #725
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, no comments
|
great, thanks. |
xav-db
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
xav-db
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…d, and Fly.io validation (#732) ## Summary - Hardens the HQL compiler by replacing panics with proper error messages - Optimizes property comparisons in WHERE clauses with new `PropertyEq`/`PropertyNeq` operators - Enhances `helix check` to validate generated Rust code with cargo check - Adds Fly.io deployment guardrails (name length, duplicate app detection) ## Key Changes - **Compiler robustness**: 20+ panic/unreachable calls replaced with descriptive errors (E627, E628, E109) - **Optimized codegen**: Inner traversals like `_::{field}::EQ(var::{field})` now generate direct property access - **Better validation**: Schema fields checked for duplicates (case-insensitive) and reserved names - **CLI improvements**: `helix check` now runs cargo check; offers GitHub issue creation on failure - **Fly.io safeguards**: 32-char app name limit, underscore→hyphen conversion, existing app detection ## Related Issues Closes #719 (filepath issue) Closes #718 (inner traversals) Closes #725 (filepath tagging) Closes #728, #729 (various fixes) <!-- greptile_comment --> <h2>Greptile Overview</h2> <h3>Greptile Summary</h3> This PR significantly improves the HelixQL compiler's robustness, performance, and user experience through several key enhancements. The main changes replace 20+ panic/unreachable calls with proper error handling (E627, E628, E109), optimize property comparisons in WHERE clauses with new `PropertyEq`/`PropertyNeq` operators that generate direct `get_property()` calls instead of inefficient `G::from_iter` traversals, and enhance the `helix check` command to validate generated Rust code with cargo check. Additionally, the PR adds comprehensive Fly.io deployment guardrails including name length validation and duplicate app detection, along with GitHub issue creation functionality for improved error reporting. <details><summary><h3>Important Files Changed</h3></summary> | Filename | Score | Overview | |----------|-------|----------| | helix-db/src/helixc/analyzer/methods/traversal_validation.rs | 5/5 | Added PropertyEq/PropertyNeq optimization for simple property traversals and improved error handling | | helix-db/src/helixc/generator/bool_ops.rs | 5/5 | Implemented PropertyEq and PropertyNeq boolean operators with optimized code generation | | helix-cli/src/commands/check.rs | 4/5 | Complete rewrite adding cargo check validation and GitHub issue reporting capabilities | | helix-db/src/helixc/analyzer/methods/schema_methods.rs | 4/5 | Added duplicate field name validation and extended reserved field names | | helix-cli/src/github_issue.rs | 4/5 | New module for automated GitHub issue creation with cargo error parsing | | helix-cli/src/commands/integrations/fly.rs | 4/5 | Added comprehensive Fly.io deployment validation and safeguards | | helix-db/src/helixc/parser/schema_parse_methods.rs | 5/5 | Fixed filepath reporting bug and replaced unreachable calls with proper error handling | | helix-db/src/helixc/analyzer/error_codes.rs | 5/5 | Added new error codes E109, E627, E628 with comprehensive documentation | | helix-db/src/helixc/analyzer/methods/graph_step_validation.rs | 5/5 | Replaced panics with E627 errors for shortest path validation | | helix-db/src/helixc/analyzer/methods/statement_validation.rs | 5/5 | Replaced panic with E628 error for invalid DROP usage | | helix-cli/src/commands/build.rs | 5/5 | Made core compilation functions module-public for reuse in check command | | helix-cli/src/utils.rs | 5/5 | Added HQL content collection function for GitHub issue reporting | | helix-db/src/helixc/parser/expression_parse_methods.rs | 5/5 | Replaced unreachable calls with descriptive error messages | | helix-db/src/helixc/parser/graph_step_parse_methods.rs | 5/5 | Enhanced error handling for order_by and shortest path parsing | | helix-db/src/helixc/parser/traversal_parse_methods.rs | 5/5 | Replaced unreachable calls with proper error handling | | helix-db/src/helixc/generator/traversal_steps.rs | 5/5 | Added support for PropertyEq/PropertyNeq in WhereRef Display implementation | | helix-db/src/helixc/analyzer/methods/infer_expr_type.rs | 4/5 | Added type validation for vector identifiers in AddV and SearchV operations | | helix-db/src/helixc/parser/utils.rs | 5/5 | Replaced unreachable macro with proper error handling | | helix-db/src/helixc/parser/types.rs | 5/5 | Fixed error handling by replacing unreachable with fallback case | | helix-db/src/helixc/analyzer/utils.rs | 5/5 | Added "ID" to reserved identifiers list | | hql-tests/tests/user_test_2/queries.hx | 5/5 | New test file demonstrating optimized inner traversal patterns | | hql-tests/tests/user_test_2/schema.hx | 5/5 | GitHub-style schema for testing property comparison optimizations | | hql-tests/tests/user_test_3/queries.hx | 5/5 | Document processing queries for RAG use cases | | hql-tests/tests/user_test_3/schema.hx | 5/5 | Document processing schema with chunks and embeddings | | helix-cli/src/tests/check_tests.rs | 5/5 | Updated test functions to include MetricsSender parameter | | helix-cli/Cargo.toml | 5/5 | Added open crate dependency for browser functionality | | hql-tests/test.sh | 4/5| Updated paths from helix-db to helix-db-core structure | | README.md | 5/5 | Updated X/Twitter handle from @hlx_db to @HelixDB | | CONTRIBUTORS.md | 5/5 | Updated X/Twitter handle for consistency | | helix-cli/src/lib.rs | 5/5 | Added github_issue module declaration | | helix-cli/src/main.rs | 5/5 | Added github_issue import and metrics_sender to check command | | hql-tests/tests/user_test_2/helix.toml | 5/5 | Configuration for WHERE filter test project | | hql-tests/tests/user_test_3/helix.toml | 5/5 | Configuration for document processing test project | </details> <details><summary><h3>Sequence Diagram</h3></summary> ```mermaid sequenceDiagram participant User participant CLI as "helix check" participant ProjectContext as "Project Context" participant Compiler as "HQL Compiler" participant Cargo as "cargo check" participant GitHub as "GitHub Issue" participant Browser User->>CLI: "helix check [instance]" CLI->>ProjectContext: "Load helix.toml" ProjectContext-->>CLI: "Project configuration" CLI->>CLI: "Validate syntax first" CLI->>Compiler: "Parse .hx files" Compiler-->>CLI: "Syntax validation result" alt Syntax valid CLI->>CLI: "Ensure helix repo cached" CLI->>CLI: "Prepare instance workspace" CLI->>Compiler: "Compile project to Rust" Compiler-->>CLI: "Generated queries.rs" CLI->>CLI: "Copy files to helix-repo-copy" CLI->>Cargo: "cargo check on generated code" Cargo-->>CLI: "Compilation result" alt Cargo check fails CLI->>CLI: "Filter errors only" CLI->>User: "Show cargo check failed" CLI->>User: "Offer to create GitHub issue" alt User accepts issue creation CLI->>CLI: "Collect .hx content" CLI->>CLI: "Build GitHub issue URL" CLI->>Browser: "Open GitHub issue page" Browser-->>User: "Pre-filled issue form" end else Cargo check succeeds CLI->>User: "Check completed successfully" end else Syntax invalid CLI->>User: "Syntax errors with diagnostics" end ``` </details> <!-- greptile_other_comments_section --> **Context used:** - Context from `dashboard` - Main documentation for all of HelixDB, the SDKs, HelixQL, and the Helix CLI ([source](https://app.greptile.com/review/custom-context?memory=861865e1-0cf9-4602-a499-40c0c360142c)) - Context from `dashboard` - readme for helixdb ([source](https://app.greptile.com/review/custom-context?memory=128beba4-b562-4fe4-b7ba-aef1f5b3f204)) <!-- /greptile_comment -->
Description
Helixdb cli tool when running helix push dev correctly detects the error but tags the wrong file. so this PR resolves this issue
Related Issues
Closes #719
Checklist when merging to main
rustfmthelix-cli/Cargo.tomlandhelixdb/Cargo.tomlAdditional Notes
Here is a image that shows new output

Greptile Overview
Greptile Summary
Fixed incorrect file path reporting in duplicate schema definition errors by ensuring the
namefield location includes the filepath for both nodes and edges.Key Changes:
parse_node_defto usepair.loc_with_filepath(filepath.clone())instead ofpair.loc()for the node name location (line 25)parse_edge_defto usepair.loc_with_filepath(filepath.clone())instead ofpair.loc()for the edge name location (line 501)edge.name.0ornode.name.0, theLocstruct contains the correctfilepathfieldImpact:
schema.hxwere incorrectly reported as being inqueries.hxImportant Files Changed
File Analysis
loc_with_filepath()instead ofloc(), ensuring error messages correctly report the file where duplicate definitions occur.Sequence Diagram
sequenceDiagram participant CLI as helix push dev participant Parser as HelixParser participant Schema as parse_node_def/parse_edge_def participant Analyzer as check_duplicate_schema_definitions participant Error as Error Reporter CLI->>Parser: Parse schema files Parser->>Schema: parse_edge_def(pair, filepath) Note over Schema: OLD: name: (pair.loc(), name)<br/>NEW: name: (pair.loc_with_filepath(filepath), name) Schema-->>Parser: EdgeSchema with name location Parser->>Schema: parse_node_def(pair, filepath) Note over Schema: OLD: name: (pair.loc(), name)<br/>NEW: name: (pair.loc_with_filepath(filepath), name) Schema-->>Parser: NodeSchema with name location Parser->>Analyzer: check_duplicate_schema_definitions Note over Analyzer: Detects duplicate edge "Has_product" Analyzer->>Error: push_schema_err(edge.name.0, ...) Note over Error: edge.name.0 now contains correct filepath<br/>(schema.hx instead of queries.hx) Error-->>CLI: Display error with correct file path