-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add a way to skip generating some sql type definitions #4002
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.
Thanks for submitting this PR 👍
I'm generally open to such a change but I would expect that it includes at least a few tests + passes the CI. Also we likely want to have a changelog entry for this addition as it might be useful for others as well?
Wouldn't updating PgVector be a simpler option that would avoid:
? |
@Ten0 I fear it's not possible for crates like pgvetcor to do that in a helpful way. They need to define their own SQL type for:
I do not see how you could one or the other with arbitrary types provided by a downstream crate (from point of view of pgvector) without running into limitations due to the orphan rule. For reference their diesel implementation. I would assume that this also affects other crates like postgis-diesel |
Thanks, I added a test and a CHANGELOG entry. but I struggle with the test. It feels like the diesel.toml in the test folder isn't read. PS: I also renamed the command to except_custom_type_definitions to match the table filters. |
Ahh, right. I always forget this. The tests do 2 things:
Sorry for not mentioning that earlier. |
CHANGELOG.md
Outdated
@@ -12,7 +12,7 @@ Increasing the minimal supported Rust version will always be coupled at least wi | |||
|
|||
### Added | |||
|
|||
* Support `[print_schema] skip_missing_sql_type_definitions=["Vector"]`. If a `missing type` matches one element on the list it's skipped. | |||
* Support `[print_schema] exclude_custom_sql_type_definitions=["Vector"]`. If a `missing type` matches one element on the list it's skipped. |
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.
You name it except_custom_sql_type_definitions
* Support `[print_schema] exclude_custom_sql_type_definitions=["Vector"]`. If a `missing type` matches one element on the list it's skipped. | |
* Support `[print_schema] except_custom_sql_type_definitions=["Vector"]`. If a `missing type` matches one element on the list it's skipped. |
@@ -10,6 +10,14 @@ fn run_infer_schema_without_docs() { | |||
test_print_schema("print_schema_simple_without_docs", vec![]); | |||
} | |||
|
|||
#[test] | |||
fn run_except_custom_type_definitions() { |
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.
This test is run for all backends but it only contains files for postgresql. That's the reason it is failing for SQLite. There are two options to fix this:
- Add the relevant test files for the other backends (SQLite, MySQL)
- add a
#[cfg(feature = "postgres")]
to the test function to mark this test as postgres only
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 fine for me now beside the unrelated changes in the Changelog.md file.
I will merge this likely next week to give @Ten0 the possibility to response to my answer above.
CHANGELOG.md
Outdated
* Added automatic usage of all sqlite `rowid` aliases when no explicit primary key is defined for `print-schema` | ||
* Added a `#[dsl::auto_type]` attribute macro, allowing to infer type of query fragment functions | ||
* Added the same type inference on `Selectable` derives, which allows skipping specifying `select_expression_type` most of the time, in turn enabling most queries to be written using just a `Selectable` derive. | ||
* Added an optional `#[diesel(skip_insertion)]` field attribute to the `Insertable` derive macro, allowing fields which map to generated columns to be skipped during insertion. | ||
* Added the same type inference on `Selectable` derives, which allows skipping specifying `select_expression_type` most |
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.
I would rather like to not have all these unrelated changes in this file.
Makes sense, didn't realize the types were so non-specific so yeah the resulting types are significantly more correct this way 👍😊 |
Thanks for your changes, any clue why the CI is failing again ? |
The CI errors are caused by rust-lang/rust#124396. They are not related to this PR. I will fix them as soon as I'm back from holiday. |
I run across a problem using some third party crates. with
generate_missing_sql_type_definitions
So I added a
skip_missing_sql_type_definitions
, it takes a vec of regexes if any matches we ignore the type.I hope this is valuable.