[gwt-contrib] Re: code review requested for 1.5, issues 2836, 2894, 2911 - various I18NSync issues

[John Tamplin]

After further FTF discussion, committed to releases/1.5 at r3756.

-- 
John A. Tamplin
Software Engineer (GWT), Google

--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---

[gwt-contrib] Re: code review requested for 1.5, issues 2836, 2894, 2911 - various I18NSync issues

[John Tamplin]

On Tue, Oct 14, 2008 at 3:41 PM, Alex Rudnick <[EMAIL PROTECTED]> wrote:

> The code looks good (one typographical nit), but I'm pretty sure you
> should change the comments in the tests that reference local files on
> your machine. Also, all the test files should have copyright notices
> (don't they?), but it looks like you took some of them out.
>
> Let me know if I'm confused about the copyright notices.
>

The existing code that was checked in had copyright notices.  However, they
are generated files, which typically don't have them - they were manually
added before when we went through all of the files and added copyright
headers.  To maintain that, we would need to have I18NSync generate a header
(and since it is used by external users that would need to be configurable
as they probably wouldn't like us putting a Google copyright notice on their
generated fiels) or manually edit them.


> Particular notes:
>
> user/src/com/google/gwt/i18n/rebind/AbstractLocalizableInterfaceCreator.java
> 89: Probably don't need a dash in "hexa-decimal".
>
> user/test/com/google/gwt/i18n/client/gen/Colors.java
> Why no copyright/license notice at the top?
>

Generated code, as mentioned above.


> Also, most people don't have a /usr/local/google/home/jat directory handy.
>

It seems useful in general to have the code generator include a comment
about what it was generated from.  However, I agree that isn't really
appropriate here.  Maybe all these generated files should be manually edited
for checkin -- otherwise we probably need to add options to I18NSync to
specify the header and whether to include the source file name.  Another
option would be to strip off part of the path, such as from a classpath root
so it would just get com/google/gwt/i18n/client/gen/*.properties.

-- 
John A. Tamplin
Software Engineer (GWT), Google

--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---

[gwt-contrib] Re: code review requested for 1.5, issues 2836, 2894, 2911 - various I18NSync issues

[Alex Rudnick]

Alright, taking a look!

The code looks good (one typographical nit), but I'm pretty sure you
should change the comments in the tests that reference local files on
your machine. Also, all the test files should have copyright notices
(don't they?), but it looks like you took some of them out.

Let me know if I'm confused about the copyright notices.

Particular notes:
user/src/com/google/gwt/i18n/rebind/AbstractLocalizableInterfaceCreator.java
89: Probably don't need a dash in "hexa-decimal".

user/test/com/google/gwt/i18n/client/gen/Colors.java
Why no copyright/license notice at the top?

Also, most people don't have a /usr/local/google/home/jat directory handy.

user/test/com/google/gwt/i18n/client/gen/TestMessages.java:
Again, took out the copyright notice?

user/test/com/google/gwt/i18n/client/gen/SingleMessages.java:
No copyright notice, reference to /usr/local/google/home/jat

user/test/com/google/gwt/i18n/client/gen/TestMessagesQuoting.java:
Reference to /usr/local/google/home/jat

user/test/com/google/gwt/i18n/client/gen/TestBadKeys.java:
user/test/com/google/gwt/i18n/client/gen/TestConstantsQuoting.java:
user/test/com/google/gwt/i18n/client/gen/Shapes.java:
user/test/com/google/gwt/i18n/client/gen/SingleConstant.java:

No copyright notice, reference to /usr/local/google/home/jat

On Tue, Oct 14, 2008 at 2:05 PM, John Tamplin <[EMAIL PROTECTED]> wrote:
> Please review this change for 1.5.3 which fixes a number of small issues.
> The patch is larger than you might expect because it includes parts copied
> from elsewhere (and then simplified) for choosing what characters to quote
> and how to quote them, but at this point I felt it was safer to copy them
> than to try and refactor the code to make it usable here.  Tests were
> corrected (previously the output could be nondeterministic in the case of
> duplicated keys in the properties file, now it is deterministic which
> required some changes to tests which only happened to work before) and new
> tests added.  The last part of the bulk is the updating the generated files
> to match the output of I18NSync.
>
> Toby, I would like you to look at the ClassLoader changes to I18NSync.java
> -- you can ignore the rest if you like.
>
> This was tested by running I18NSyncTest_ (which is not included in our test
> suite) and then running I18NSuite which runs all the tests using the
> generated code.
>
> --
> John A. Tamplin
> Software Engineer (GWT), Google
>

-- 
Alex Rudnick
swe, gwt, atl

--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---