Re: [Savannah-hackers-public] Savane i18n (step 2.5)

[Assaf Gordon]

Hi,

> On Jul 14, 2017, at 02:34, Ineiev <address@hidden> 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