-
Notifications
You must be signed in to change notification settings - Fork 75
Implicit char to string v2 #1420
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
base: master
Are you sure you want to change the base?
Conversation
…ataFrame.convert()
…sing, failing when the returned type is Char or String
…ing parsing, failing when the returned type is Char or String
c0d17a6
to
3211749
Compare
…s a char converter is passed explicitly
…generally a better explanation of how convertTo works
f785179
to
6d8d40a
Compare
6d8d40a
to
a29f5c6
Compare
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.
For me mostly clear, the great idea to provide PR not only with tests, but doc changes - it's really help to jump into a logic
* `Instant` (kotlinx.datetime, kotlin.time, and java.time) | ||
* `enum` classes (by name) | ||
|
||
Note that converting between `Char` and `Int` is done by ASCII character code. |
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.
Let's make it as a Warning or NOTE - it's a very important fact
} | ||
|
||
@Test | ||
fun `parse to Char`() { |
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.
Should it be an opposite test, which has columnOf('a', 'b')
col.parse().type() shouldBe typeOf<String>()?
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.
No, that no longer happens. columnOf('a', 'b').parse()
throws IllegalStateException because it cannot be parsed to any other type than Char
. You could write tryParse()
but then the result would still be of type Char
/** | ||
* Tries to parse a column of chars as strings into a column of a different type. | ||
* Each parser in [Parsers] is run in order until a valid parser is found, | ||
* a.k.a. that parser was able to parse all values in the column successfully. If a parser |
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.
Great explanation!
Fixes #998 and surpasses #999.
(Can either be for
Beta3 orBeta4, whatever suits us best)We again have three parts, but slightly modified from #999:
String
-fallback forChar
inconvert
We get
String
-fallback forChar
columns indf.convert().to<>()
... andcol.convertTo<X>()
. This means you can now do things like:Note: We do keep the old
convert
behavior ofChar
->Int
by ASCII code. This is in line with the JVM world. We have parsing now if you want theChar
'1'
to turn into theInt
1
.Converting from
String
columns can only result in enum instances (like above), value class instances, or callparse()
, so we follow the same behavior forChar
columns now.Char
parsingdf.parse()
now not only parses string columns, but also char ones (this can be changed if you don't agree, of course).We also gain:
Here I deviate from #999, as
because the parsing didn't do anything. However similar to
String
columns, there'stryParse()
which can return the same type as the input.convertTo<> { parser {} }
Char
supportFixes #998 by trying out all provided
String
converters ifChar
s are encountered without a converter.I also updated the docs and expanded upon
convertTo
a lot to prevent confusion, like what was experienced in #998.