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

Make primary_type return the class type vs string #325

Merged
merged 2 commits into from
Feb 26, 2019

Conversation

Blacksmoke16
Copy link
Contributor

Makes the primary_type method return the actual .class type of the primary key, versus a string. Having the actual type is much more useful.

spec/granite/table/table_spec.cr Show resolved Hide resolved
@@ -44,7 +44,7 @@ module Granite::Table
@@table_name = "{{table_name}}"
@@primary_name = "{{primary_name}}"
@@primary_auto = "{{primary_auto}}"
@@primary_type = "{{primary_type}}"
@@primary_type = {{primary_type}}
Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with this, and I agree it is a better paradigm. It is also a breaking change to the API.

Copy link
Contributor Author

@Blacksmoke16 Blacksmoke16 Feb 25, 2019

Choose a reason for hiding this comment

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

We could add a new method to get the actual type and leave the string one untouched if we wanted to avoid the breaking change. However, I'm skeptical if it would be worth it?

Copy link
Member

Choose a reason for hiding this comment

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

I have no problem incrementing the version, just want to me sure it's marked as a pull which is a breaking change.

Copy link
Member

@robacarp robacarp left a comment

Choose a reason for hiding this comment

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

thanks @Blacksmoke16

@Blacksmoke16 Blacksmoke16 merged commit db6b5ed into amberframework:master Feb 26, 2019
@Blacksmoke16 Blacksmoke16 deleted the primary_type-class branch February 26, 2019 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants