http://gwt-code-reviews.appspot.com/72802/diff/2001/3006 File dev/core/src/com/google/gwt/dev/GwtVersion.java (right):
http://gwt-code-reviews.appspot.com/72802/diff/2001/3006#newcode30 Line 30: implements Comparable<ReleaseNumber> { What's the value of the ReleaseType enum? Where do we ever actually check for Milestone v. RC v. Other? Seems like ReleaseNumber would be much simpler without it, and maybe not even be necessary. If ReleaseType and ReleaseNumber are all about the -rc and -ms bits, why not simply allow some arbitrary string canonicalized via .toLower() as part of the version object? If it all really is necessary, it seems pretty fragile. If I read this right a typo on the suffix makes it a release rather than failing. We have rules but we don't enforce them. I guess I'm unclear on what is actually being fixed with this patch. I tuned out of the version thread before it ended. Having that info in the patch description (and the svn commit message) would be helpful. http://gwt-code-reviews.appspot.com/72802/diff/2001/3006#newcode157 Line 157: private static final String DEFAULT_NO_NAG_VERSION = "0.0.999"; Seems like all these 999s should be a constant. http://gwt-code-reviews.appspot.com/72802/diff/2001/3006#newcode169 Line 169: // TODO(jat): make final Why TODO and not now? http://gwt-code-reviews.appspot.com/72802/diff/2001/3006#newcode251 Line 251: public boolean isReleased() { Is this actually used anywhere in our code? If not, and there is no comparable existing public API that we have to keep parity with, I'd remove it. http://gwt-code-reviews.appspot.com/72802/diff/2001/3001 File dev/core/test/com/google/gwt/dev/GwtVersionTest.java (right): http://gwt-code-reviews.appspot.com/72802/diff/2001/3001#newcode55 Line 55: v2 = new GwtVersion("0.0.0b"); Why is it good that these are equal? http://gwt-code-reviews.appspot.com/72802 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
