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

DM-42257: Make the text type un-sized so length is not required #53

Merged
merged 5 commits into from Apr 17, 2024

Conversation

JeremyMcCormick
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick commented Apr 12, 2024

The length should not be required when defining a text column, since it represents an unbounded string. The is_sized flag in Felis forces a column with that type to require a length, which we do not want for this datatype.

@JeremyMcCormick
Copy link
Collaborator Author

@gpdf proposes changing this line to "char" instead of "unicodeChar" on this PR.

class Unicode(FelisType, felis_name="unicode", votable_name="unicodeChar", is_sized=True):

@JeremyMcCormick
Copy link
Collaborator Author

@gpdf proposes changing this line to "char" instead of "unicodeChar" on this PR.

class Unicode(FelisType, felis_name="unicode", votable_name="unicodeChar", is_sized=True):

@gpdf I've changed this to "char" on this PR for the Felis text type.

@JeremyMcCormick
Copy link
Collaborator Author

@andy-slac I incorporated a number of your unrelated changes (though not all) from #51 in this.

Copy link
Contributor

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

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

Looks great! And I mentioned it already - we need unit tests for everything to cover all type-related quirks.

@JeremyMcCormick
Copy link
Collaborator Author

we need unit tests for everything to cover all type-related quirks

I'll keep this in mind and add more on another ticket/branch.

There are a decent number of tests for the type system now but maybe not covering all corner cases.

@gpdf
Copy link

gpdf commented Apr 17, 2024

@gpdf proposes changing this line to "char" instead of "unicodeChar" on this PR.

class Unicode(FelisType, felis_name="unicode", votable_name="unicodeChar", is_sized=True):

@gpdf I've changed this to "char" on this PR for the Felis text type.

The actual change you made was on the class Text() line, not on class Unicode(), and that's correct, that's exactly what I had in mind: fix Text() and leave Unicode() alone.

Copy link

@gpdf gpdf left a comment

Choose a reason for hiding this comment

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

Thanks, looks good.

@JeremyMcCormick
Copy link
Collaborator Author

@gpdf proposes changing this line to "char" instead of "unicodeChar" on this PR.

class Unicode(FelisType, felis_name="unicode", votable_name="unicodeChar", is_sized=True):

@gpdf I've changed this to "char" on this PR for the Felis text type.

The actual change you made was on the class Text() line, not on class Unicode(), and that's correct, that's exactly what I had in mind: fix Text() and leave Unicode() alone.

My mistake on this comment - I actually changed the line with class Text and not class Unicode.

@JeremyMcCormick JeremyMcCormick merged commit 2c982cf into main Apr 17, 2024
9 checks passed
@JeremyMcCormick JeremyMcCormick deleted the tickets/DM-42257 branch April 17, 2024 22:49
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

3 participants