-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Remove As
methods on ColumnType.
#1864
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
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.
return conv.GetStringConversion<T>(type); | ||
|
||
bool identity; | ||
// Second choice: if key, utilize the KeyValues metadata for that key, if it has one and is text. | ||
if (schema.HasKeyValues(col, type.KeyCount)) | ||
if (schema.HasKeyValues(col, keyType.KeyCount)) |
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.
KeyCount [](start = 49, length = 8)
Cool. Could change this to Count
I think now that you've made this a key type, but not required. That would just be something not quite in the scope of the PR of course, so it's fine not to do it yet, I think.
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 think we can address this when remove KeyCount from the base ColumnType.
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.
Fantastic @eerhardt ! I'm surprised that this set of methods involved the change of only about 100 some odd lines, but so much the better.
Remove the following methods from ColumnType:
More PRs will be coming in this form. But this seemed like the lowest hanging fruit to start making incremental progress here.
Part of the work necessary for #1860