On 01/01/2016 03:33 PM, Rostislav Krasny wrote:
> Hi there,
> 
> After pulling the latest changes in the "next" branch of the Fred
> project I found a few issues in BaseL10n and in BaseL10nTest. Please
> take a look at my bug report that also includes a patch:
> 
> https://bugs.freenetproject.org/view.php?id=6782
> 
> I build Fred by Maven, so I converted the structure of the code
> directories into the Maven standard. And my patch is based in it.
> However my patch would be interesting to the original Fred developers
> as well because it fixes issues in BaseL10n that are not Maven/Ant
> related. It also fixes code portability, meaning it should not be
> Ant-only and should continue to work properly and pass tests after
> rearranged into Maven source directories structure.
> 
> P.S.
> I would like show you one of the test changes that demonstrates
> improvements done in the BaseL10n :
> 
> -        // it would be nice to handle this correctly, but it seems
> like more trouble than it's worth
> -        //assertEquals("<tag>content <tag>nested</tag></tag>",
> node.generateChildren());
> -        assertEquals("test.selfNestedSubstitution", node.generateChildren());
> +        assertEquals("<tag>content <tag>nested</tag></tag>",
> node.generateChildren());

Hi Rostislav,

Thanks for the patch! A few thoughts:

- this mixes formatting, style, and functional changes, which should be
  avoided.
- the commit message should enumerate the behavior changes and explain
  why they were made if it's not painfully obvious.
- after 1471 is released we're planning to move to Gradle (and use
  gradle-witness), but the directory structure changes will be
  equivalent. At that point we will certainly merge something similar
  to your changes (if not your changes outright) to restore sanity to
  the BaseL10nTest resource path stuff.

We usually do code review on GitHub; could you please open this as a
pull request? [0]

- Steve

[0] https://github.com/freenet/fred/pulls

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Devl mailing list
[email protected]
https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl

Reply via email to