Skip to content
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

Support int8 migration versions via new int8-versions feature #330

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mpalmer
Copy link
Contributor

@mpalmer mpalmer commented May 3, 2024

This addresses, though does not really fix #83, because it doesn't make refinery support timestamped migrations by default, but only if you opt-in to the new feature. However, making it an optional feature neatly sidesteps the unanswered questions in the issue, and so makes the implementation easier to complete and land.

This addresses, though does not really *fix* rust-db#83, because it doesn't make refinery support timestamped migrations *by default*, but only if you opt-in to the new feature.
However, making it an optional feature neatly sidesteps the unanswered questions in the issue, and so makes the implementation easier to complete and land.
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, and thanks for your interest! As I stated on #83 (comment) I'd rather this was not feature gated and instead a function was provided to migrate to int8. Would you be willing to do that instead?

Thanks

Comment on lines +10 to +14
#[cfg(not(feature = "int8-versions"))]
pub type SchemaVersion = i32;
#[cfg(feature = "int8-versions")]
pub type SchemaVersion = i64;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

features in Rust should be additive not mutually exclusive, enabling --all-features should not break compilation, see here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling --all-features doesn't, as far as I'm aware, break compilation. CI is all green.

@mpalmer
Copy link
Contributor Author

mpalmer commented May 20, 2024

Would you be willing to do that instead?

Not at the present time, no. As you identified in #83, there are unanswered design questions with migrating existing users to an int8 column, I don't use the CLI, and the approach in this PR Works For Me(TM). Perhaps one of the commenters from #83, such as @ankhers, might like to use this PR as a basis?

@jxs jxs force-pushed the int8-versions branch 3 times, most recently from fb26681 to 666cbd3 Compare August 14, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants