Added import test to fix import for parse_rc #97
Conversation
icnocop
force-pushed
the
import_parser_rc
branch
from
c457474 to
0ad5675
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
lib/Serge/Engine/Plugin/parse_rc.pm
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
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.
t/engine.t
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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?
t/engine.t
Outdated
| @@ -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'); | |||
There was a problem hiding this comment.
See my comment above for var name refactoring.
icnocop
force-pushed
the
import_parser_rc
branch
2 times, most recently
from
69caeba to
dbd2f39
Compare
|
I've updated the PR with your code comments and added #98 for the typo and refactoring as suggested. Thank you. |
icnocop
force-pushed
the
import_parser_rc
branch
2 times, most recently
from
8ea2de9 to
01fe5ad
Compare
|
When I took out passing Is that expected given the input file application.rc? If so, can you explain why, please? All the keys in the When I pass |
lib/Serge/Engine/Plugin/parse_rc.pm
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I created PR #99 with just this single-line change.
Thank you.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
icnocop
force-pushed
the
import_parser_rc
branch
from
01fe5ad to
5831284
Compare
Fixed parsing rc file and import test
icnocop
force-pushed
the
import_parser_rc
branch
from
2b70f1e to
b260bde
Compare
Fixes #92