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

Treat bom-only CSV file as empty #112

Merged
merged 4 commits into from
Dec 6, 2022

Conversation

KengoTODA
Copy link
Contributor

Hello, thanks for making your awesome product open and contribution-welcome! 🙌

Today I found that the CSV file that contains only BOM is NOT treated as empty; It should return listOf() but it doesn't.
Here I want to suggest a small change to fix this issue, could you check it when you have time?

Thanks in advance! 👋

Signed-off-by: Kengo TODA <toda_k@henry.jp>
Signed-off-by: Kengo TODA <toda_k@henry.jp>

fun readLineWithTerminator(): String? {
val sb = StringBuilder()
do {
val c = br.read()

if (c == -1) {
if (sb.isEmpty()) {
if (sb.isEmpty() || (sb.length == 1 && sb[0] == BOM)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This implementation does not care in which line we found the BOM.
In other words, if second line contains BOM, this implementation treats the line as empty and return null.

I think this is enough for this class, but if necessary it is possible to add private var isFirstLine: Boolean to return null only when we read the first line.

diff --git a/src/commonMain/kotlin/com/github/doyaaaaaken/kotlincsv/client/BufferedLineReader.kt b/src/commonMain/kotlin/com/github/doyaaaaaken/kotlincsv/client/BufferedLineReader.kt
index beee976..4251199 100644
--- a/src/commonMain/kotlin/com/github/doyaaaaaken/kotlincsv/client/BufferedLineReader.kt
+++ b/src/commonMain/kotlin/com/github/doyaaaaaken/kotlincsv/client/BufferedLineReader.kt
@@ -6,6 +6,8 @@ package com.github.doyaaaaaken.kotlincsv.client
 internal class BufferedLineReader(
     private val br: Reader
 ) {
+    private var isFirstLine: Boolean = true
+
     companion object {
         private val BOM = '\uFEFF'
     }
@@ -16,7 +18,8 @@ internal class BufferedLineReader(
             val c = br.read()
 
             if (c == -1) {
-                if (sb.isEmpty() || (sb.length == 1 && sb[0] == BOM)) {
+                if (sb.isEmpty() || (isFirstLine && sb.length == 1 && sb[0] == BOM)) {
+                    isFirstLine = false
                     return null
                 } else {
                     break
@@ -43,6 +46,7 @@ internal class BufferedLineReader(
                 break
             }
         } while (true)
+        isFirstLine = false
         return sb.toString()
     }
 

Copy link
Collaborator

Choose a reason for hiding this comment

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

This implementation does not care in which line we found the BOM.

I agree with this policy.

Signed-off-by: Kengo TODA <toda_k@henry.jp>
Signed-off-by: Kengo TODA <toda_k@henry.jp>
Copy link
Collaborator

@doyaaaaaken doyaaaaaken left a comment

Choose a reason for hiding this comment

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

Hi @KengoTODA, thank you for your first contribution! 😄

I completely agree with your proposal!
I have made a few comments, please check them out.

}

private fun StringBuilder.isEmptyLine(): Boolean =
this.isEmpty() || this.length == 1 && this[0] == BOM
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code below seems to better grasp the intent.
this.isEmpty() || (this.length == 1 && this[0] == BOM)

Copy link
Contributor Author

@KengoTODA KengoTODA Dec 6, 2022

Choose a reason for hiding this comment

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

I prefer it too, but CodeFactor did not like it so I fixed it as 5231efb 😇

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh... 😇 OK, Let's leave it as is!

@doyaaaaaken doyaaaaaken merged commit a3cf01e into jsoizo:master Dec 6, 2022
@doyaaaaaken
Copy link
Collaborator

Thanks for contribution! Released on v1.7.0.

@KengoTODA KengoTODA deleted the toda_k/empty-with-bom branch December 7, 2022 02:16
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.

2 participants