Added import test to fix import for parse_rc #97

icnocop · 2018-11-18T05:59:57Z

Fixes #92

codecov-io · 2018-11-18T06:32:48Z

Codecov Report

Merging #97 into master will increase coverage by 1.04%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
+ Coverage   52.05%   53.09%   +1.04%     
==========================================
  Files          91       91              
  Lines        6901     6904       +3     
  Branches     1730     1731       +1     
==========================================
+ Hits         3592     3666      +74     
+ Misses       2645     2561      -84     
- Partials      664      677      +13

Impacted Files	Coverage Δ
lib/Serge/Importer.pm	`42.85% <100%> (+40.37%)`	⬆️
lib/Serge/Engine/Plugin/parse_rc.pm	`78.33% <50%> (+2.89%)`	⬆️
lib/Serge/Engine.pm	`60.73% <0%> (ø)`	⬆️
lib/Serge/Engine/Job.pm	`87.87% <0%> (+1.01%)`	⬆️
lib/Serge/Plugin/Base.pm	`86.36% <0%> (+4.54%)`	⬆️
lib/Serge/Interface/PluginHost.pm	`94.44% <0%> (+16.66%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa5ac6b...eb7a379. Read the comment docs.

iafan · 2018-12-21T03:43:16Z

lib/Serge/Engine/Plugin/parse_rc.pm

@@ -86,7 +86,7 @@ sub parse {
        if ($orig_str) {
            my $str = $orig_str;
            $str =~ s/""/"/g;
-            $translated_str = &$callbackref($str, undef, $hint, undef, $lang);
+            $translated_str = &$callbackref($str, undef, $hint, undef, $lang, $hint);


This is not a proper fix. The last parameter is supposed to be a unique key (if available in the file), whereas you just use hint as a key there (which can be constructed in the parser in multiple ways).

I suggest moving this change out of the importer test PR.

iafan · 2018-12-21T03:44:33Z

t/engine.t

@@ -9,7 +9,7 @@ use strict;
 # script or assign them to the environment variable SERGE_ENGINE_TESTS as a
 # comma-separated list.  The following two examples are equivalent:
 #
-# perl t/enigne.t parse_json parse_strings
+# perl t/engine.t parse_json parse_strings


Can you externalize all typos / var name refactoring in a smaller PR that we can merge separately and which won't distract from the main importer PR?

iafan · 2018-12-21T03:45:02Z

t/engine.t

@@ -42,7 +42,8 @@ sub Serge::Engine::file_mtime {
    return 12345678;
 }

-my $thisdir = dirname(abs_path(__FILE__));
+my $this_dir = dirname(abs_path(__FILE__));
+my $tests_dir = catfile($this_dir, 'data', 'engine');


See my comment above for var name refactoring.

icnocop · 2018-12-21T04:57:33Z

I've updated the PR with your code comments and added #98 for the typo and refactoring as suggested.

Thank you.

icnocop · 2018-12-21T05:15:05Z

When I took out passing $hint as the last parameter to $callbackref, the test fails with the error:

Parser plugin didn't provide a key value in a callback. Importing translations with this plugin is not possible, unless you use `--disambiguate-keys` option.

Is that expected given the input file application.rc?

If so, can you explain why, please?

All the keys in the STRINGTABLE are unique, so I don't expect any disambiguity.

When I pass $idstr as the last parameter to $callbackref, the test fails with errors like:

Duplicate key 'BEGIN' found in source file

iafan · 2018-12-21T05:20:44Z

lib/Serge/Engine/Plugin/parse_rc.pm

@@ -86,7 +86,7 @@ sub parse {
        if ($orig_str) {
            my $str = $orig_str;
            $str =~ s/""/"/g;
-            $translated_str = &$callbackref($str, undef, $hint, undef, $lang);
+            $translated_str = &$callbackref($str, undef, $hint, undef, $lang, $idstr);


Now, this is a proper fix, and I'll be happy to merge the PR with this single-line change.

As for the importer, right now it is just a copy-paste of t/engine.t, and requires writing some additional tests; it also doesn't seem to do the actual import as the reference translations table in your test is empty. Would be more interesting to somehow hook the importing mode right into engine.t, where it would first generate the translations (and check them against reference files, and then use importer to import translations back and see that the reference import database matches the generated one.

I created PR #99 with just this single-line change.

Thank you.

Hi @iafan.

I updated the PR with another commit to fix more issues with import and parse_rc.
The translations table in the reference output are now populated correctly.

I agree that we should try to re-use some of the code in engine.t to consolidate the duplicate code this PR creates.
I'm not sure how to implement it as you suggested yet, but I'll try to at least get one step closer.

I'm wondering if you want me to create a new PR with just the changes in Importer.pm and parse_rc.pm?

Thank you.

In regards to "use importer to import translations back and see that the reference import database matches the generated one.", do you expect there to be a new reference-output database to check against the import? For example, .\t\data\engine\parse_rc\00\reference-output\imported-database? The properties and translations tables seem to be the only ones that are different.

Fixes serge-community#92

Fixed parsing rc file and import test

icnocop force-pushed the import_parser_rc branch from c457474 to 0ad5675 Compare November 18, 2018 06:30

iafan reviewed Dec 21, 2018

View reviewed changes

icnocop force-pushed the import_parser_rc branch 2 times, most recently from 69caeba to dbd2f39 Compare December 21, 2018 04:12

icnocop force-pushed the import_parser_rc branch 2 times, most recently from 8ea2de9 to 01fe5ad Compare December 21, 2018 05:01

iafan reviewed Dec 21, 2018

View reviewed changes

Added import test to fix import for parse_rc

5831284

Fixes serge-community#92

icnocop force-pushed the import_parser_rc branch from 01fe5ad to 5831284 Compare March 8, 2019 02:46

Use source path when importing

b260bde

Fixed parsing rc file and import test

icnocop force-pushed the import_parser_rc branch from 2b70f1e to b260bde Compare March 8, 2019 11:56

icnocop added 2 commits March 8, 2019 17:33

More fixes to parse_rc.pm

eb7a379

Revert to using output path when importing

8346d54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added import test to fix import for parse_rc #97

Added import test to fix import for parse_rc #97

icnocop commented Nov 18, 2018

codecov-io commented Nov 18, 2018 •

edited

Loading

iafan Dec 21, 2018

iafan Dec 21, 2018

iafan Dec 21, 2018

icnocop commented Dec 21, 2018 •

edited

Loading

icnocop commented Dec 21, 2018 •

edited

Loading

iafan Dec 21, 2018

icnocop Dec 21, 2018

icnocop Mar 8, 2019

icnocop Mar 14, 2019

Added import test to fix import for parse_rc #97

Are you sure you want to change the base?

Added import test to fix import for parse_rc #97

Conversation

icnocop commented Nov 18, 2018

codecov-io commented Nov 18, 2018 • edited Loading

Codecov Report

iafan Dec 21, 2018

Choose a reason for hiding this comment

iafan Dec 21, 2018

Choose a reason for hiding this comment

iafan Dec 21, 2018

Choose a reason for hiding this comment

icnocop commented Dec 21, 2018 • edited Loading

icnocop commented Dec 21, 2018 • edited Loading

iafan Dec 21, 2018

Choose a reason for hiding this comment

icnocop Dec 21, 2018

Choose a reason for hiding this comment

icnocop Mar 8, 2019

Choose a reason for hiding this comment

icnocop Mar 14, 2019

Choose a reason for hiding this comment

codecov-io commented Nov 18, 2018 •

edited

Loading

icnocop commented Dec 21, 2018 •

edited

Loading

icnocop commented Dec 21, 2018 •

edited

Loading