-
Notifications
You must be signed in to change notification settings - Fork 76
add Path overloads for io methods #1560
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
| public fun DataFrame.Companion.read(path: Path, header: List<String> = emptyList()): AnyFrame = | ||
| read( | ||
| path = path, | ||
| format = guessFormat(path.toString())?.also { |
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 doesn't use guessFormatForExtension() anymore, does it? the logic has changed a bit
|
|
||
| class GuessOverloadsTest { | ||
|
|
||
| private fun sampleJson(): 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.
Can do with a @Language("JSON") :)
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.
also, const val
| """.trimIndent() | ||
|
|
||
| @Test | ||
| fun read_guess_overloads_String_Path_File_produce_same_df() { |
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 is this snake_case btw? XD usually we write test names with backticks and spaces
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.
Junie, is this you? ;P
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.
You need proper junie guide))
| class HtmlOverloadsTest { | ||
|
|
||
| @Test | ||
| fun writeHtml_overloads_String_Path_File_produce_same_content() { |
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.
same
| html.writeHtml(fileOut) | ||
| html.writeHtml(strOut) | ||
|
|
||
| fun readText(p: Path): String = String(Files.readAllBytes(p), Charsets.UTF_8) |
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.
uhm, you can just call path.readText() in Kotlin. This function is useless
| import kotlin.io.path.createTempDirectory | ||
| import kotlin.io.path.writeText | ||
|
|
||
| class CsvOverloadsTest { |
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 doesn't add much value
|
|
||
| @Test | ||
| fun writeExcel_overloads_String_Path_File_create_nonempty_files() { | ||
| val df = dataFrameOf("name", "age")( |
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.
you can @Suppress("ktlint:standard:argument-list-wrapping") to make it look better. Or use the "name" to columnOf() api
| fun GeoDataFrame<*>.writeGeoJson(path: Path) { | ||
| val featureJSON = FeatureJSON() | ||
| file.outputStream().use { outputStream -> | ||
| Files.newOutputStream(path).use { outputStream -> |
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.
why not use the kotlin api XD path.outputStream()
| directory.mkdirs() | ||
| /** Path overload for writing Shapefile to a directory */ | ||
| fun GeoDataFrame<*>.writeShapefile(directory: Path) { | ||
| if (!Files.exists(directory)) { |
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.
also this has Kotlin extension functions, similar to File
|
|
||
| return isOpenApiStr(file.readText()) | ||
| public fun isOpenApi(path: Path): Boolean { | ||
| val name = path.fileName?.toString()?.lowercase() ?: return false |
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.
path also has .extension, just like files
| val yaml = tmp.resolve("api.yml") | ||
| yaml.writeText("openapi: 3.0.0\ncomponents:\n schemas: {}\n") | ||
| val json = tmp.resolve("api.json") | ||
| json.writeText("{" + "\"openapi\":\"3.0.0\",\"components\":{\"schemas\":{}}}") |
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's this +?
zaleslaw
left a comment
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.
- Naming in tests
- @BeforeClass if it's possible
- Clear the point about deleting files with Comparator
- Clear the code from unexpected stuff
|
|
||
| class JsonOverloadsTest { | ||
|
|
||
| private fun sampleJson(): 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.
Create @before or @BeforeAll and move initialization there
https://www.baeldung.com/junit-before-beforeclass-beforeeach-beforeall
Why it's important?
- better structured
- performant (run once before all tests, not for each test)
- best practice for Kotlin
| writeJson(file.toPath(), prettyPrint) | ||
| } | ||
|
|
||
| public fun AnyFrame.writeJson(path: Path, prettyPrint: Boolean = false) { |
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 improve here! No public method without KDoc
| } | ||
| } | ||
|
|
||
| fun GeoDataFrame<*>.writeShapefile(directory: File) { |
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.
Could you please say, are these write functions part of public API or not? I guess it's a public and should be with apropriate modifier and KDocs?
| } | ||
|
|
||
| fun GeoDataFrame<*>.writeGeoJson(file: File) { | ||
| // TODO: adds ids that breaks order of reading |
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.
TODO: issue or it will be forgotten
|
|
||
| class CsvOverloadsTest { | ||
|
|
||
| private fun sampleCsv(): 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.
Looks weird to me, better to prepare test data in @BeforeClass section and reuse it
| compression = compression, | ||
| adjustCsvSpecs = ADJUST_CSV_SPECS, | ||
| ) | ||
| } |
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.
Sorry, but from this edits with +/- is not clear, did you removed a adjustCsvSpecs = ADJUST_CSV_SPECS parameter? and what's happened with compression = Compression.of(file.toPath()),?
| r.rowsCount() shouldBe df.rowsCount() | ||
| r.columnNames() shouldBe df.columnNames() | ||
| } finally { | ||
| Files.walk(tmp).sorted(Comparator.reverseOrder()).forEach { it.toFile().delete() } |
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's happened if it will be an exception in finally block? on File.delete()
Do we really need to clear the tmp directory in this way?
| } | ||
|
|
||
| @Test | ||
| fun ipc_overloads_write_and_read() { |
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.
Rename testname to back stick
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.
*backtick XD
| try { | ||
| val file = tmp.resolve("schema.json").toFile().also { it.writeText("{}") } | ||
| val path = file.toPath() | ||
| val str = path.toUri().toString() // String-оверлоад ожидает URL/URI-строку |
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.
Das ist nicht auf Russisch geschrieben, aber auf eine translatenend und machine-generiert subset von Russisch!
Helps #527 - adds missing
Pathoverloads