-
Notifications
You must be signed in to change notification settings - Fork 17
Feature gated struct fields #322
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
base: master
Are you sure you want to change the base?
Conversation
c04f1c7 to
5d9a571
Compare
5c1af39 to
295a230
Compare
295a230 to
c8d05f4
Compare
| if name == *"pub_time" { | ||
| // Temporary solution to skip pub_time field in butane tests | ||
| continue; | ||
| } |
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 is causing the CI failure when running with the datetime feature enabled.
As this macro is being run with the cfg's not evaluated and thus fields removed, the .table files contain all of the fields, causing a mismatch with the ORM objects.
I think we need to extract the cfgs, and evaluate them explicitly.
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.
It looks like there isnt any built in Rust support for this. c.f. rust-lang/rust#90765
Maybe a crate which can help...
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.
https://github.com/EmbarkStudios/cfg-expr looks like it can help.
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.
CARGO_CFG_FEATURE is only available in build.rs , so this approach would require that there is a build.rs, and it saves CARGO_CFG_FEATURE somewhere so that the migration code can know which features will be active during compilation.
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.
An alternative is to add a "cfg attributes" member to AColumn , so that the .table files describe the fact that some columns are conditional based on features.
This sounds more sensible, as then the .table file doesnt change based on which features are enabled.
However this will have some strange effects on migrations.
If a migration is created with a feature gated field enabled, and then compile without that feature, butane will be wanting to create a new migration to remove the field. While migration creation is an explicit user action, it may have been months since they created the feature gated field, and may not realise why the migration is trying to delete a field.
I am thinking we might want to put the active feature list on disk under .butane, and then warn users if they try to generate a migration using a different feature set.
Closes #150
IGNORE PR ATM - it can only be used to show the problem by running