Merge pull request #47169 from AndyBarcia/completeCSVErrors

Improve error reporting when parsing CSV translation files.

core/io/file_access.cpp

@@ -441,6 +441,11 @@ Vector<String> FileAccess::get_csv_line(const String &p_delim) const {
 			current += c;
 		}
 	}
+
+	if (in_quote) {
+		WARN_PRINT(vformat("Reached end of file before closing '\"' in CSV file '%s'.", get_path()));
+	}
+
 	strings.push_back(current);
 
 	return strings;

editor/import/resource_importer_csv_translation.cpp

@@ -107,18 +107,17 @@ Error ResourceImporterCSVTranslation::import(const String &p_source_file, const
 		translations.push_back(translation);
 	}
 
-	line = f->get_csv_line(delimiter);
-
-	while (line.size() == locales.size() + 1) {
+	do {
+		line = f->get_csv_line(delimiter);
 		String key = line[0];
 		if (!key.is_empty()) {
+			ERR_FAIL_COND_V_MSG(line.size() != locales.size() + 1, ERR_PARSE_ERROR, vformat("Error importing CSV translation: expected %d locale(s), but the '%s' key has %d locale(s).", locales.size(), key, line.size() - 1));
+
 			for (int i = 1; i < line.size(); i++) {
 				translations.write[i - 1]->add_message(key, line[i].c_unescape());
 			}
 		}
-
-		line = f->get_csv_line(delimiter);
-	}
+	} while (!f->eof_reached());
 
 	for (int i = 0; i < translations.size(); i++) {
 		Ref<Translation> xlt = translations[i];

tests/core/io/test_file_access.h

@@ -38,25 +38,27 @@
 namespace TestFileAccess {
 
 TEST_CASE("[FileAccess] CSV read") {
-	Ref<FileAccess> f = FileAccess::open(TestUtils::get_data_path("translations.csv"), FileAccess::READ);
+	Ref<FileAccess> f = FileAccess::open(TestUtils::get_data_path("testdata.csv"), FileAccess::READ);
 	REQUIRE(!f.is_null());
 
 	Vector<String> header = f->get_csv_line(); // Default delimiter: ",".
-	REQUIRE(header.size() == 3);
+	REQUIRE(header.size() == 4);
 
 	Vector<String> row1 = f->get_csv_line(","); // Explicit delimiter, should be the same.
-	REQUIRE(row1.size() == 3);
+	REQUIRE(row1.size() == 4);
 	CHECK(row1[0] == "GOOD_MORNING");
 	CHECK(row1[1] == "Good Morning");
 	CHECK(row1[2] == "Guten Morgen");
+	CHECK(row1[3] == "Bonjour");
 
 	Vector<String> row2 = f->get_csv_line();
-	REQUIRE(row2.size() == 3);
+	REQUIRE(row2.size() == 4);
 	CHECK(row2[0] == "GOOD_EVENING");
 	CHECK(row2[1] == "Good Evening");
 	CHECK(row2[2].is_empty()); // Use case: not yet translated!
 	// https://github.com/godotengine/godot/issues/44269
 	CHECK_MESSAGE(row2[2] != "\"", "Should not parse empty string as a single double quote.");
+	CHECK(row2[3] == "\"\""); // Intentionally testing only escaped double quotes.
 
 	Vector<String> row3 = f->get_csv_line();
 	REQUIRE(row3.size() == 6);

tests/core/string/test_translation.h

@@ -151,7 +151,7 @@ TEST_CASE("[OptimizedTranslation] Generate from Translation and read messages")
 }
 
 #ifdef TOOLS_ENABLED
-TEST_CASE("[Translation] CSV import") {
+TEST_CASE("[TranslationCSV] CSV import") {
 	Ref<ResourceImporterCSVTranslation> import_csv_translation = memnew(ResourceImporterCSVTranslation);
 
 	HashMap<StringName, Variant> options;
@@ -163,17 +163,39 @@ TEST_CASE("[Translation] CSV import") {
 	Error result = import_csv_translation->import(TestUtils::get_data_path("translations.csv"),
 			"", options, nullptr, &gen_files);
 	CHECK(result == OK);
-	CHECK(gen_files.size() == 2);
+	CHECK(gen_files.size() == 4);
+
+	TranslationServer *ts = TranslationServer::get_singleton();
 
 	for (const String &file : gen_files) {
 		Ref<Translation> translation = ResourceLoader::load(file);
 		CHECK(translation.is_valid());
-		TranslationServer::get_singleton()->add_translation(translation);
+		ts->add_translation(translation);
 	}
 
-	TranslationServer::get_singleton()->set_locale("de");
+	ts->set_locale("en");
 
-	CHECK(Object().tr("GOOD_MORNING", "") == "Guten Morgen");
+	// `tr` can be called on any Object, we reuse TranslationServer for convenience.
+	CHECK(ts->tr("GOOD_MORNING") == "Good Morning");
+	CHECK(ts->tr("GOOD_EVENING") == "Good Evening");
+
+	ts->set_locale("de");
+
+	CHECK(ts->tr("GOOD_MORNING") == "Guten Morgen");
+	CHECK(ts->tr("GOOD_EVENING") == "Good Evening"); // Left blank in CSV, should source from 'en'.
+
+	ts->set_locale("ja");
+
+	CHECK(ts->tr("GOOD_MORNING") == String::utf8("おはよう"));
+	CHECK(ts->tr("GOOD_EVENING") == String::utf8("こんばんは"));
+
+	/* FIXME: This passes, but triggers a chain reaction that makes test_viewport
+	 * and test_text_edit explode in a billion glittery Unicode particles.
+	ts->set_locale("fa");
+
+	CHECK(ts->tr("GOOD_MORNING") == String::utf8("صبح بخیر"));
+	CHECK(ts->tr("GOOD_EVENING") == String::utf8("عصر بخیر"));
+	*/
 }
 #endif // TOOLS_ENABLED
 

tests/data/testdata.csv

@@ -0,0 +1,8 @@
+Header 1,Header 2,Header 3,Header 4
+GOOD_MORNING,"Good Morning","Guten Morgen","Bonjour"
+GOOD_EVENING,"Good Evening","",""""""
+Without quotes,"With, comma","With ""inner"" quotes","With ""inner"", quotes"","" and comma","With ""inner
+split"" quotes and
+line breaks","With \nnewline chars"
+Some other~delimiter~should still work, shouldn't it?
+What about	tab separated	lines, good?

tests/data/translations.csv

@@ -1,8 +1,3 @@
-keys,en,de
-GOOD_MORNING,"Good Morning","Guten Morgen"
-GOOD_EVENING,"Good Evening",""
-Without quotes,"With, comma","With ""inner"" quotes","With ""inner"", quotes"","" and comma","With ""inner
-split"" quotes and
-line breaks","With \nnewline chars"
-Some other~delimiter~should still work, shouldn't it?
-What about	tab separated	lines, good?
+keys,en,de,ja,fa
+GOOD_MORNING,"Good Morning","Guten Morgen","おはよう","صبح بخیر"
+GOOD_EVENING,"Good Evening","","こんばんは","عصر بخیر"