LGTM

I didn't look through the sample templates src files closely, but I'm
assuming they haven't materially changed.


http://gwt-code-reviews.appspot.com/1371808/diff/1/user/src/com/google/gwt/user/tools/WebAppCreator.java
File user/src/com/google/gwt/user/tools/WebAppCreator.java (right):

http://gwt-code-reviews.appspot.com/1371808/diff/1/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode154
user/src/com/google/gwt/user/tools/WebAppCreator.java:154:
templates.remove("eclipse");
Can you add a comment explaining why these are removed?  If they are
incompatible, can we log a warning.

http://gwt-code-reviews.appspot.com/1371808/diff/1/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode214
user/src/com/google/gwt/user/tools/WebAppCreator.java:214:
templates.remove("ant");
I think a warning or even an error would be appropriate for conflicting
options.

http://gwt-code-reviews.appspot.com/1371808/diff/1/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode330
user/src/com/google/gwt/user/tools/WebAppCreator.java:330:
templates.add(template);
Will add duplicates.  I recommend using a Set for templates.

http://gwt-code-reviews.appspot.com/1371808/diff/1/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode352
user/src/com/google/gwt/user/tools/WebAppCreator.java:352: + ",
sourceName=" + sourceName + "isBinary=" + isBinary + "]";
"isBinary=" => ", isBinary="

http://gwt-code-reviews.appspot.com/1371808/diff/1/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode363
user/src/com/google/gwt/user/tools/WebAppCreator.java:363:
spaces

http://gwt-code-reviews.appspot.com/1371808/diff/1/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode403
user/src/com/google/gwt/user/tools/WebAppCreator.java:403: private
ArrayList<String> templates = new ArrayList<String>();
A Set might be better to avoid duplicates.

http://gwt-code-reviews.appspot.com/1371808/diff/1/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode511
user/src/com/google/gwt/user/tools/WebAppCreator.java:511: // maven
project. Maven projects should include libs as maven dependencies.
Are you sure its safe to remove the !maven check?

http://gwt-code-reviews.appspot.com/1371808/

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

Reply via email to