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

Add enum column support to ActiveRecordColumns #1000

Merged
merged 3 commits into from
Jul 7, 2022

Conversation

q3aiml
Copy link
Contributor

@q3aiml q3aiml commented Jun 17, 2022

Motivation

Handle enum columns so they do not fall back to untyped. Primarily I want this so that the type includes T.nilable when appropriate which does not happen when it is untyped.

Implementation

I added support for ActiveRecord::Enum::EnumType in ActiveRecordColumnTypeHelper. The setter method's type supports all coercible types. This requires inspecting the underlying column type to differentiate between numeric and non-numeric backing columns.

Tests

Added coverage in existing test for generating proper types for every ActiveRecord column type.

Handle and specify String type so it does not fall back to untyped.
@KaanOzkan KaanOzkan requested a review from a team June 21, 2022 19:33
Copy link
Collaborator

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -225,6 +225,8 @@ def title?; end
t.datetime :datetime_column
t.decimal :money_column
t.text :serialized_column
# Ideally this would test t.enum but that is not supported by sqlite
t.string :enum_column
Copy link
Member

Choose a reason for hiding this comment

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

This is not a valid database column given the example below. The test model constructs a default enum which maps to integer fields, so the backing column should be integer as well. Otherwise, the enum column would not work properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I updated this to test both integer and string backing columns and adjusted the associated model setup.

@ghost ghost added the cla-needed label Jun 27, 2022
@q3aiml q3aiml requested a review from paracycle June 29, 2022 21:59
@q3aiml
Copy link
Contributor Author

q3aiml commented Jun 29, 2022

Thanks for the reviews! CLA should be coming through in the next day or two.

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. We are good to merge once the CLA is in place! 🙏

@q3aiml
Copy link
Contributor Author

q3aiml commented Jul 5, 2022

CLA should now be signed. I don't see a way for me to trigger the check to re-run other than pushing a change to verify, but I will keep an eye out in case there is some issue.

Thanks for the great contribution experience. Reviewing and providing good feedback on community PRs is a big commitment from your side and I really appreciate it.

@Morriar
Copy link
Collaborator

Morriar commented Jul 6, 2022

👋 Hi @q3aiml,

You can try:

$ git commit --amend --no-edit

To push again without edit.

Allow Symbol and Integer as appropriate based on underlying column. Fix
associated spec to test both integer and string columns and use proper
associated enum setup.
@q3aiml q3aiml force-pushed the type-active-record-enum-columns branch from c397367 to 38cf4b0 Compare July 6, 2022 13:52
@ghost ghost removed the cla-needed label Jul 6, 2022
@paracycle paracycle merged commit 9f5d897 into Shopify:main Jul 7, 2022
@shopify-shipit shopify-shipit bot temporarily deployed to production July 14, 2022 18:56 Inactive

sig { params(column_type: ActiveRecord::Enum::EnumType).returns(String) }
def enum_setter_type(column_type)
case column_type.subtype
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks rails < 7 because subtype was previously private. Rails commit: rails/rails@70259f5#diff-a0de22dc0114613fdc67f48c916e70b9983ef47bcc84b96232c36398af1d166fR156-R159

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.

None yet

5 participants