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

[Ineiev]

On Fri, Jul 14, 2017 at 09:50:13PM -0400, Assaf Gordon wrote:
> 
> If I may suggest/comment on few things:

Thanks!

> 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.

Just to make it clear for me: $GLOBALS is a way to access variables
from global scope. $ffeedback is one of such variables, and it's
in fact per-request. The new variable is used instead of $ffeedback
in a similar way.

> 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

From the previous revision, just one line of code remained,
it's not copyrightable.

> 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

The target in the makefile that extracts localized strings scans
for files ending in .php and .class, but a few PHP files had
no extension. I added links to make the target work.

Probably I should write a more detailed commit log.

> 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?

Not really: it exposes those values for i18n. actually, we may want
to store them in hashes.txt rather than in the database,
like our license list (but then we'll need to rework some code).

> 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).

I'll try to separate them.

> 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 ?

I wasn't sure it worked. if it does, we could drop the protocol part instead.

> 7.
> In the following commit:
> https://git.savannah.gnu.org/cgit/administration/savane.git/commit/?h=i18n-2017-07-13&id=caeeafb6eecebee4fc3517773813d3db0d2d2148
...
> Would be nice to explain what was fixed, or (since it isn't in 'master' yet) 
> - rebase+squash.

I wanted to to avoid forced pushes in that branch.

I'll push another one.

signature.asc
Description: Digital signature