-
Notifications
You must be signed in to change notification settings - Fork 61
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 an option to read Excel cell values as a String regardless of their content type #745
Conversation
@@ -83,6 +84,9 @@ private fun setWorkbookTempDirectory() { | |||
/** | |||
* @param sheetName sheet to read. By default, the first sheet in the document | |||
* @param columns comma separated list of Excel column letters and column ranges (e.g. “A:E” or “A,C,E:F”) | |||
* @param formattingOptions range of columns to read as String regardless of a cell type. |
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.
It's not just that right? You can format a range of columns as string using different options as defined by a DataFormatter
. The description should probably reflect this.
If this argument was just to format columns as string, the argument should be colRangeAsString
or better, mirror the colTypes
argument from CSV.
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.
Changed the parameter for some overloads to StringColumns class and provided see also where it's still FormattingOptions
dataframe-excel/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/xlsx.kt
Show resolved
Hide resolved
dataframe-excel/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/xlsx.kt
Outdated
Show resolved
Hide resolved
0703ed9
to
2e97bd2
Compare
@@ -83,6 +84,8 @@ private fun setWorkbookTempDirectory() { | |||
/** | |||
* @param sheetName sheet to read. By default, the first sheet in the document | |||
* @param columns comma separated list of Excel column letters and column ranges (e.g. “A:E” or “A,C,E:F”) | |||
* @param stringColumns range of columns to read as String regardless of a cell type. | |||
* For example, by default numeric cell with value "3" will be parsed as Double with value being 3.0. With this option, it will be simply "3" |
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'd surround types with []
to make them clickable :). *their type or *a cell's type
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.
What types? String and Double?
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.
yes, that's what I do to all types across the kdocs :) I think it's nice for interactivity and consistency.
dataframe-excel/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/xlsx.kt
Outdated
Show resolved
Hide resolved
dataframe-excel/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/xlsx.kt
Show resolved
Hide resolved
} | ||
|
||
/** | ||
* @param sheet sheet to read. | ||
* @param columns comma separated list of Excel column letters and column ranges (e.g. “A:E” or “A,C,E:F”) | ||
* @param formattingOptions range of columns to read as String regardless of a cell type. |
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.
This should probably say "as [String], formatted by the given [FormattingOptions]" or something like that. Otherwise it's not clear what the difference is by the overload with stringColumns and this one.
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 "see also" below and the fact that type is different is enough
2e97bd2
to
46b90b3
Compare
…ir content type fixes #669
46b90b3
to
c980e81
Compare
fixes #669
Once again based on user feedback, and also was mentioned previously