Hi, > On Jul 14, 2017, at 02:34, Ineiev <ine...@gnu.org> wrote: > > I haven't reviewed i18n of all frontend sources, but the branch > has grown long enough, and it contains some changes unrelated to > i18n itself (most notably, preview for tracker comments); so > I'd like to fast-forward master to i18n-2017-07-13 and install it.
From a cursory look these look good, please go ahead. I guess that if any bugs pop-up (e.g. with the VCS refactoring or preview) we'll fix them as we go along. If I may suggest/comment on few things: 1. This commit introduces a $GLOBAL variable: https://git.savannah.gnu.org/cgit/administration/savane.git/commit/?h=i18n-2017-07-13&id=ea1a4f617e8e7f30f1a693da9b4a67ef941ff312 It is not explained (in git or code comments) what is this for. Is this a per-user-session variable? if so, is $GLOBAL the best place for it? So far, to my limited understanding, PHP $GLOBALS have been used solely for configuration items, and then they have initialization in the "includes" PHP and the "/etc/savanah/.savanh.conf.php" file. 2. This is perhaps a stylistic issue, but I wouldn't remove other people's copyright notice from files, even if most of the code they wrote is gone. They did work on the file... https://git.savannah.gnu.org/cgit/administration/savane.git/commit/?h=i18n-2017-07-13&id=42db98526decb7e96acd35937677981efc31e8d3 3. What is the purpose of this commit? I could not figure out from the minimal code, and there are no comments: https://git.savannah.gnu.org/cgit/administration/savane.git/commit/?h=i18n-2017-07-13&id=4ec7cc50553196ade45947073b7b80bca1a58cc0 4. This is a very large commit: https://git.savannah.gnu.org/cgit/administration/savane.git/commit/?h=i18n-2017-07-13&id=dc86c0388fc8fccd19cc22a4ffb50bb8378be177 But somewhere in the middle it contains the following: ==== + $job_categories_as_of_2017_06 = array(_("None"), + _("Developer"), + _("Project Manager"), + _("Unix Admin"), + _("Doc Writer"), + _("Tester"), + _("Support Manager"), + _("Graphic/Other Designer"), + _("Translator"), + _("Other")); ==== Isn't that hard-coding values that are in the database? This seems bit unclean... 5. There are several very large commits titled "review localized strings", e.g. https://git.savannah.gnu.org/cgit/administration/savane.git/commit/?h=i18n-2017-07-13&id=dc86c0388fc8fccd19cc22a4ffb50bb8378be177 https://git.savannah.gnu.org/cgit/administration/savane.git/commit/?h=i18n-2017-07-13&id=dc86c0388fc8fccd19cc22a4ffb50bb8378be177 https://git.savannah.gnu.org/cgit/administration/savane.git/commit/?h=i18n-2017-07-13&id=bd382290a6a4a23afbb3e530c565cbb224ad2739 They touch thousands of lines, and most of the changed lines have nothing to do with localization (e.g. reformatting/breaking lines, changing indentation, changing upper to lower case, adding HTML tags like <p>). I must admit I only skimmed through them. It would be easier to understand if those were broken down per topic (i.e. if "localization" - then only touching lines that actually use localized text, and and having separate commits for breaking lines / re-indentation / etc). 6. in this commit: https://git.savannah.gnu.org/cgit/administration/savane.git/commit/?h=i18n-2017-07-13&id=ee576350973364b3c226225ac3bccae65f7174c7 If we're touching the URL for savane's source code, perhaps consider changing to HTTPS ? 7. In the following commit: https://git.savannah.gnu.org/cgit/administration/savane.git/commit/?h=i18n-2017-07-13&id=caeeafb6eecebee4fc3517773813d3db0d2d2148 since it fixes something with the previous commit, and haven't been pushed to 'master', it would be nice to rebase+squash to make it easier to understand the commits. 8. This commit has subject "fix 7e9b991e90": http://git.savannah.gnu.org/cgit/administration/savane.git/commit/?h=i18n-2017-07-13&id=292c35e4d159850d63ca263f970b3388118cee45 Would be nice to explain what was fixed, or (since it isn't in 'master' yet) - rebase+squash. regards, -assaf
