Ignore empty columns and rows, fixes #47

koral-- · koral-- · commit 589ac1c2ebcb · 2020-01-30T03:06:37.000+01:00
diff --git a/.travis.yml b/.travis.yml
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,7 @@
+### 1.0.19
+- Filter out empty rows and columns, fixes XLS(X) parsing errors - [#47](https://github.com/koral--/android-gradle-localization-plugin/issues/47)
+- Dependency versions bump
+
 ### 1.0.18
 - Add option to evaluate formulas in XLS(X) files - [#45](https://github.com/koral--/android-gradle-localization-plugin/pull/45)
 - Dependency versions bump
diff --git a/README.md b/README.md
@@ -25,7 +25,7 @@ Generation has to be invoked as additional gradle task. Java 1.8 is required.
 In whichever `build.gradle` file.
 ```groovy
 plugins {
-  id 'pl.droidsonroids.localization' version '1.0.17'
+  id 'pl.droidsonroids.localization' version '1.0.19'
 }
 ```
 Note: exact version number must be specified, `+` cannot be used as wildcard.
@@ -40,8 +40,8 @@ Note: exact version number must be specified, `+` cannot be used as wildcard.
          jcenter()
      }
      dependencies {
-         classpath 'com.android.tools.build:gradle:3.4.2'
-         classpath 'pl.droidsonroids.gradle.localization:android-gradle-localization-plugin:1.0.17'
+         classpath 'com.android.tools.build:gradle:3.5.3'
+         classpath 'pl.droidsonroids.gradle.localization:android-gradle-localization-plugin:1.0.19'
      }
  }
  ```
@@ -158,7 +158,7 @@ ignored if `useAllSheets` is set to true
 
 #### Advanced options:
 * `ignorableColumns` - default=`[]`, columns from that list will be ignored during parsing. List should
-contain column names e.g. `['Section', 'Notes']`
+contain column names e.g. `['Section', 'Notes']`. Columns containing only empty cells are always ignored.
 * `allowNonTranslatableTranslation` - default=`false`, if set to true resources marked
 non-translatable but translated are permitted
 * `allowEmptyTranslations` - default=`false`, if set to true then empty values are permitted
diff --git a/build.gradle b/build.gradle
@@ -24,13 +24,13 @@ repositories {
 
 dependencies {
     implementation gradleApi()
-    implementation 'org.codehaus.groovy:groovy:2.5.8'
+    implementation 'org.codehaus.groovy:groovy:2.5.9'
     implementation 'org.marketcetera.fork:commons-csv:3.2.0'
     implementation 'org.apache.poi:poi-ooxml-schemas:4.1.1'
     implementation 'org.apache.poi:poi-ooxml:4.1.1'
     implementation 'org.apache.poi:poi:4.1.1'
     implementation 'org.apache.poi:ooxml-schemas:1.4'
-    testImplementation 'junit:junit:4.12'
+    testImplementation 'junit:junit:4.13'
     testImplementation 'org.assertj:assertj-core:3.14.0'
     testImplementation 'org.xmlunit:xmlunit-core:2.6.3'
 }
diff --git a/gradle.properties b/gradle.properties
@@ -1,4 +1,4 @@
-VERSION_NAME=1.0.18
+VERSION_NAME=1.0.19
 GROUP=pl.droidsonroids.gradle.localization
 
 POM_DESCRIPTION=Gradle plugin for generating localized string resources
diff --git a/gradle/wrapper/gradle-wrapper.jar b/gradle/wrapper/gradle-wrapper.jar
diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties
@@ -1,5 +1,5 @@
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-6.0.1-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-6.1.1-bin.zip
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists
diff --git a/src/main/groovy/pl/droidsonroids/gradle/localization/ParserEngine.groovy b/src/main/groovy/pl/droidsonroids/gradle/localization/ParserEngine.groovy
@@ -65,8 +65,12 @@ class ParserEngine {
     void parseSpreadsheet() {
         mCloseableInput.withCloseable {
             Map<String, String[][]> sheets = mParser.getResult()
-            for (String sheetName: sheets.keySet()) {
+            for (String sheetName : sheets.keySet()) {
                 String[][] allCells = sheets.get(sheetName)
+
+                def shouldIgnoreEmptyHeaderCell = mConfig.ignorableColumns.contains("")
+                Utils.validateColumnEmptiness(allCells, shouldIgnoreEmptyHeaderCell)
+
                 String outputFileName
                 if (sheetName != null) {
                     outputFileName = sheetName + ".xml"
@@ -96,7 +100,7 @@ class ParserEngine {
                 def arrays = new HashMap<String, List<StringArrayItem>>()
                 for (i in 1..cells.length - 1) {
                     String[] row = cells[i]
-                    if (row == null) {
+                    if (row == null || row.every { it.isEmpty() }) {
                         continue
                     }
                     if (row.length < sourceInfo.mColumnsCount) {
@@ -108,7 +112,7 @@ class ParserEngine {
                     }
 
                     String name = row[sourceInfo.mNameIdx]
-                    def value = row[j]
+                    String value = row[j]
 
                     String comment = null
                     if (sourceInfo.mCommentIndex >= 0 && !row[sourceInfo.mCommentIndex].empty) {
diff --git a/src/main/groovy/pl/droidsonroids/gradle/localization/SourceInfo.groovy b/src/main/groovy/pl/droidsonroids/gradle/localization/SourceInfo.groovy
@@ -14,10 +14,6 @@ class SourceInfo {
     }
 
     SourceInfo(String[] headerLine, ConfigExtension config, File resDir, String outputFileName) {
-        if (headerLine == null || headerLine.length < 2) {
-            throw new IllegalArgumentException("Invalid header: $headerLine")
-        }
-
         List<String> header = Arrays.asList(headerLine)
         mColumnsCount = headerLine.length
         mBuilders = new XMLBuilder[mColumnsCount]
@@ -47,7 +43,7 @@ class SourceInfo {
             throw new IllegalStateException("Default locale column not present")
         }
 
-        def reservedColumns = [config.commentColumnName, config.translatableColumnName, config.formattedColumnName]
+        def reservedColumns = [config.commentColumnName, config.translatableColumnName, config.formattedColumnName, ""]
         if (config.tagEscapingStrategyColumnName != null) {
             reservedColumns.add(config.tagEscapingStrategyColumnName)
         }
diff --git a/src/main/groovy/pl/droidsonroids/gradle/localization/Utils.groovy b/src/main/groovy/pl/droidsonroids/gradle/localization/Utils.groovy
@@ -19,4 +19,22 @@ class Utils {
     static boolean containsHTML(String text) {
         return TAG_PATTERN.matcher(text).find() || CDATA_PATTERN.matcher(text).find()
     }
+
+    static void validateColumnEmptiness(String[][] allCells, boolean shouldIgnoreEmptyHeaderCell) {
+        def headerLine = allCells[0]
+        if (headerLine == null || headerLine.length < 2) {
+            throw new IllegalArgumentException("Invalid header: $headerLine")
+        }
+
+        if (!shouldIgnoreEmptyHeaderCell) {
+            def emptyHeaderIndices = headerLine.findIndexValues { it.empty }
+            allCells.eachWithIndex { String[] row, int rowIndex ->
+                emptyHeaderIndices.forEach { i ->
+                    if (i < row.length && !row[i.intValue()].empty) {
+                        throw new IllegalArgumentException("Not ignored column #$i contains empty header but non-empty cell at row #$rowIndex")
+                    }
+                }
+            }
+        }
+    }
 }
diff --git a/src/test/groovy/pl/droidsonroids/gradle/localization/ParseCSVTest.groovy b/src/test/groovy/pl/droidsonroids/gradle/localization/ParseCSVTest.groovy
@@ -19,13 +19,11 @@ class ParseCSVTest extends LocalizationPluginTestBase {
 
     @Test
     void testValidFile() {
-        println 'testing valid file'
         parseTestFile(RESOURCE_FOLDER + VALID_FILE_NAME)
     }
 
     @Test(expected = IllegalArgumentException.class)
     void testMissingTranslation() {
-        println 'testing invalid file'
         parseTestFile(RESOURCE_FOLDER + MISSING_TRANSLATION_FILE_NAME)
     }
 }
diff --git a/src/test/groovy/pl/droidsonroids/gradle/localization/SourceInfoTest.groovy b/src/test/groovy/pl/droidsonroids/gradle/localization/SourceInfoTest.groovy
@@ -12,21 +12,11 @@ class SourceInfoTest {
         config = new ConfigExtension()
     }
 
-    @Test(expected = IllegalArgumentException.class)
-    void testNullHeader() {
-        new SourceInfo(null, config, null)
-    }
-
     @Test(expected = IllegalArgumentException.class)
     void testEmptyHeader() {
         new SourceInfo([] as String[], config, null)
     }
 
-    @Test(expected = IllegalArgumentException.class)
-    void testTooShortHeader() {
-        new SourceInfo(['name'] as String[], config, null)
-    }
-
     @Test(expected = IllegalArgumentException.class)
     void testBothNameColumnIndexAndName() {
         config.nameColumnIndex = 1
diff --git a/src/test/groovy/pl/droidsonroids/gradle/localization/UtilsTest.groovy b/src/test/groovy/pl/droidsonroids/gradle/localization/UtilsTest.groovy
@@ -4,37 +4,60 @@ import org.junit.Test
 
 import static org.assertj.core.api.AssertionsForClassTypes.assertThat
 import static pl.droidsonroids.gradle.localization.Utils.containsHTML
+import static pl.droidsonroids.gradle.localization.Utils.validateColumnEmptiness
 
 class UtilsTest {
 
-	@Test
-	void plainTextContainsNoHTML() {
-		assertThat(containsHTML('')).isFalse()
-		assertThat(containsHTML('test')).isFalse()
-		assertThat(containsHTML('test\ntest2')).isFalse()
-		assertThat(containsHTML('<')).isFalse()
-		assertThat(containsHTML('>')).isFalse()
-		assertThat(containsHTML('<>')).isFalse()
-	}
-
-	@Test
-	void entitiesContainsNoHTML() {
-		assertThat(containsHTML('&test;')).isFalse()
-		assertThat(containsHTML('&lt;')).isFalse()
-	}
-
-	@Test
-	void textWithTagsContainsHTML() {
-		assertThat(containsHTML('<b>')).isTrue()
-		assertThat(containsHTML('<b/>')).isTrue()
-		assertThat(containsHTML('<b></b>')).isTrue()
-		assertThat(containsHTML('<b>outer1<b>inner</b>outer2</b>')).isTrue()
-	}
-
-	@Test
-	void CDATAContainsHTML() {
-		assertThat(containsHTML('<![CDATA[test<a href="http://test.test"><b>test</b></a>test]]>')).isTrue()
-		assertThat(containsHTML('<![CDATA[test]]>')).isTrue()
-		assertThat(containsHTML('<![CDATA[]]>')).isTrue()
-	}
+    @Test
+    void plainTextContainsNoHTML() {
+        assertThat(containsHTML('')).isFalse()
+        assertThat(containsHTML('test')).isFalse()
+        assertThat(containsHTML('test\ntest2')).isFalse()
+        assertThat(containsHTML('<')).isFalse()
+        assertThat(containsHTML('>')).isFalse()
+        assertThat(containsHTML('<>')).isFalse()
+    }
+
+    @Test
+    void entitiesContainsNoHTML() {
+        assertThat(containsHTML('&test;')).isFalse()
+        assertThat(containsHTML('&lt;')).isFalse()
+    }
+
+    @Test
+    void textWithTagsContainsHTML() {
+        assertThat(containsHTML('<b>')).isTrue()
+        assertThat(containsHTML('<b/>')).isTrue()
+        assertThat(containsHTML('<b></b>')).isTrue()
+        assertThat(containsHTML('<b>outer1<b>inner</b>outer2</b>')).isTrue()
+    }
+
+    @Test
+    void CDATAContainsHTML() {
+        assertThat(containsHTML('<![CDATA[test<a href="http://test.test"><b>test</b></a>test]]>')).isTrue()
+        assertThat(containsHTML('<![CDATA[test]]>')).isTrue()
+        assertThat(containsHTML('<![CDATA[]]>')).isTrue()
+    }
+
+	@Test(expected = IllegalArgumentException.class)
+    void throwsOnNullHeader() {
+		validateColumnEmptiness(new String[1][], false)
+    }
+
+	@Test(expected = IllegalArgumentException.class)
+    void throwsOnTooShortHeader() {
+		String[][] cells = new String[1][1]
+		cells[0][0] = "test"
+		validateColumnEmptiness(cells, false)
+    }
+
+	@Test(expected = IllegalArgumentException.class)
+    void throwsOnInconsistentColumn() {
+		String[][] cells = new String[2][2]
+		cells[0][0] = "test"
+		cells[0][1] = ""
+		cells[1][1] = "test"
+		cells[0][0] = "test"
+		validateColumnEmptiness(cells, false)
+    }
 }
diff --git a/src/test/resources/pl/droidsonroids/gradle/localization/valid.csv b/src/test/resources/pl/droidsonroids/gradle/localization/valid.csv
@@ -1,4 +1,4 @@
-name        ,default    ,pl         ,comment    ,translatable,formatted,tagEscapingStrategy
+name        ,default    ,pl         ,comment    ,translatable,formatted,,tagEscapingStrategy
 app         ,Application, ,,false
 cow[one]  ,cow      ,krowa
 file        ,File       ,"Plik"     ,file label ,
@@ -9,4 +9,4 @@ days[],tuesday,,,
 days[],wednesday,,,
 
 percent,%d %s,,,false,false
-tag,<b>test</b>,,,false,false,ALWAYS
+tag,<b>test</b>,,,false,false,,ALWAYS

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-VERSION_NAME=1.0.18`
	`1`	`+VERSION_NAME=1.0.19`
`2`	`2`	`GROUP=pl.droidsonroids.gradle.localization`
`3`	`3`
`4`	`4`	`POM_DESCRIPTION=Gradle plugin for generating localized string resources`
Original file line number	Diff line number	Diff line change
`@@ -14,10 +14,6 @@ class SourceInfo {`
`14`	`14`	`}`
`15`	`15`
`16`	`16`	`SourceInfo(String[] headerLine, ConfigExtension config, File resDir, String outputFileName) {`
`17`		`- if (headerLine == null \|\| headerLine.length < 2) {`
`18`		`- throw new IllegalArgumentException("Invalid header: $headerLine")`
`19`		`- }`
`20`		`-`
`21`	`17`	`List<String> header = Arrays.asList(headerLine)`
`22`	`18`	`mColumnsCount = headerLine.length`
`23`	`19`	`mBuilders = new XMLBuilder[mColumnsCount]`
`@@ -47,7 +43,7 @@ class SourceInfo {`
`47`	`43`	`throw new IllegalStateException("Default locale column not present")`
`48`	`44`	`}`
`49`	`45`
`50`		`- def reservedColumns = [config.commentColumnName, config.translatableColumnName, config.formattedColumnName]`
	`46`	`+ def reservedColumns = [config.commentColumnName, config.translatableColumnName, config.formattedColumnName, ""]`
`51`	`47`	`if (config.tagEscapingStrategyColumnName != null) {`
`52`	`48`	`reservedColumns.add(config.tagEscapingStrategyColumnName)`
`53`	`49`	`}`