-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Codegen: add ql.db_table_name
property pragma
#19063
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
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.
Pull Request Overview
This pull request adds a new property pragma, ql.db_table_name, to resolve table name conflicts in the dbscheme. Key changes include new tests for custom db_table_name handling for cpp, ql, and dbscheme generators, refactoring of pragma classes in schemadefs, and a new duplicate table‐name check in the dbscheme generator.
Reviewed Changes
Copilot reviewed 9 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
misc/codegen/test/test_cppgen.py | Adds tests validating that custom db_table_name pragmas correctly influence the generated trap names. |
misc/codegen/lib/schemadefs.py | Refactors pragma classes and introduces a parametrized approach for handling property pragmas. |
misc/codegen/test/test_dbschemegen.py | Introduces tests to catch table name conflicts and to verify that custom db_table_name pragmas override default names. |
misc/codegen/test/test_qlgen.py | Adds tests that ensure custom ql.db_table_name pragmas are applied to generated ql.Property definitions. |
misc/codegen/test/test_schemaloader.py | Updates tests to include the new ql.db_table_name behavior and adjust pragma annotations. |
misc/codegen/generators/qlgen.py | Adds a check to raise an error when a custom db_table_name pragma is provided on a single property. |
misc/codegen/generators/dbschemegen.py | Introduces a duplicate table name check that uses custom table names when provided. |
misc/codegen/generators/cppgen.py | Updates trap name generation to optionally use the custom db_table_name pragma. |
misc/codegen/generators/rustgen.py | Adjusts table name generation for properties to use the overridden db_table_name when available. |
Files not reviewed (5)
- .bazelrc: Language not supported
- .bazelrc.internal: Language not supported
- MODULE.bazel: Language not supported
- misc/codegen/.python-version: Language not supported
- misc/codegen/requirements_lock.txt: Language not supported
Comments suppressed due to low confidence (3)
misc/codegen/generators/qlgen.py:134
- The error raised when a db_table_name pragma is set on a single property should have its formatting style aligned with the project's other error messages for clarity and consistency.
if db_table_name and prop.is_single:
misc/codegen/generators/dbschemegen.py:121
- Align the duplicate table name error message with existing project conventions to ensure consistent output across generators.
raise Error(f"Duplicate table name: {name}, you can use `@ql.db_table_name` on a property to resolve this")
misc/codegen/generators/cppgen.py:43
- Verify that the custom db_table_name pragma value is applied directly when generating the trap name without unintended pluralization.
overridden_trap_name = p.pragmas.get("ql_db_table_name")
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
Nice, I ran into this on #19057, where I had to work around it by using a different name for the class. |
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.
Looks plausible to me.
This adds a
ql.db_table_name
pragma that can be used to resolve conflicts in dbscheme table names.Namely, until now if we had
both the table for the repeated properties and the table for the
FooBar
instances would get thefoo_bars
name. This would also lead to an error only once the dbscheme got loaded somewhere.Now:
codegen
ql.db_table_name("new_name")
property pragma can be used to resolve the conflict, for exampleThe python version used by bazel is also bumped to 3.12 (I couldn't care to make my code compatible with 3.11, when we're using 3.12 all over other places).
Notice that we restrict this pragma to properties only because that was easier to implement (as the table name was already a field of
ql.Property
while it wasn't one ofql.Class
) and it suffices to resolve conflicts, as they cannot arise between class tables, and a property table is always involved.As a motivating example, this is applied on Rust to undo a rename of
Path::segment
toPath::part
that was done because of this issue, getting thus closer to the official rust terminology.Commit by commit review is encouraged.