John, this seems awfully complicated, and a lot of that complixity is in support of big public API that as far as I can see is unused.
Is all of this really necessary for us to tell that 2.0.0-rc < 2.0.0, or whatever convention it was that we settled on? I also don't think this should gate release of MS1. http://gwt-code-reviews.appspot.com/72802/diff/1/5 File dev/core/src/com/google/gwt/dev/About.java (right): http://gwt-code-reviews.appspot.com/72802/diff/1/5#newcode50 Line 50: * @deprecated use {...@link #getGwtVersionObject()} or I take it getGwtVersionArray was not public api? Would library writers be relying on it? http://gwt-code-reviews.appspot.com/72802/diff/1/5#newcode97 Line 97: gwtVersionNum = tmp; Seems like gwtVersionNum field should go away, and String getGwtVersionNum() should now be gwtVersion().getNumberString(), something like that. For that matter, I wonder if GwtVersion should take over the gwtName as well? So its constructor would actually be GwtVersion(String name, String number)? (Less certain of this, please push back.) Then About becomes entirely about finding property values and cramming them into GwtVersion, where all the smarts lie. http://gwt-code-reviews.appspot.com/72802/diff/1/7 File dev/core/src/com/google/gwt/dev/GwtVersion.java (right): http://gwt-code-reviews.appspot.com/72802/diff/1/7#newcode25 Line 25: public final class GwtVersion implements Comparable<GwtVersion> { Please break out VersionComponent and ReleaseNumber into their own .java files, this is hard to read. Do they really need to be public? If they have no clients yet, they shouldn't be. And that makes me wonder if a lot of this code is unneeded. Seems like I see a lot of public methods that are never called outside of a unit test, which therefore shouldn't be committed. http://gwt-code-reviews.appspot.com/72802/diff/1/7#newcode33 Line 33: public static final ReleaseNumber RELEASE = new ReleaseNumber(ReleaseType.Release, 0); Should RELEASE be DEFAULT_RELEASE? NULL_RELEASE? Unclear from its name what its role is. http://gwt-code-reviews.appspot.com/72802/diff/1/7#newcode51 Line 51: public static ReleaseNumber parse(String value) { shouldn't be public. http://gwt-code-reviews.appspot.com/72802/diff/1/7#newcode135 Line 135: public enum ReleaseType { why public http://gwt-code-reviews.appspot.com/72802/diff/1/7#newcode187 Line 187: * soley of digits, it will be stored as an integer component instead. Why? http://gwt-code-reviews.appspot.com/72802/diff/1/7#newcode401 Line 401: public List<VersionComponent> getComponents() { I don't see any calls to this. If I didn't miss some, why is it public? Why does it exist? http://gwt-code-reviews.appspot.com/72802/diff/1/7#newcode405 Line 405: public ReleaseNumber getReleaseNumber() { ditto http://gwt-code-reviews.appspot.com/72802/diff/1/7#newcode430 Line 430: public boolean isReleased() { unused? http://gwt-code-reviews.appspot.com/72802/diff/1/3 File dev/core/test/com/google/gwt/dev/AboutTest.java (right): http://gwt-code-reviews.appspot.com/72802/diff/1/3#newcode55 Line 55: public void tstGwtVersionObject() { bad test name. This isn't running. http://gwt-code-reviews.appspot.com/72802/diff/1/2 File dev/core/test/com/google/gwt/dev/GwtVersionTest.java (right): http://gwt-code-reviews.appspot.com/72802/diff/1/2#newcode100 Line 100: * Test that GwtVersion.compareTo produced expected results. s/compareTo/equals/ This is why I think javadoc on test methods is stupid. But I digress. http://gwt-code-reviews.appspot.com/72802/diff/1/2#newcode197 Line 197: assertTrue(components.get(1).isInteger()); Seems like a lot of this kind of thing should be in a separate unit test on VersionComponent. You could get rid of a lot of these redundant isInteger checks that way w/o reducing coverage. http://gwt-code-reviews.appspot.com/72802/diff/1/2#newcode204 Line 204: assertEquals(ReleaseType.ReleaseCandidate, releaseNumber.getReleaseType()); Similarly, seems like a lot of redundant coverage of ReleaseNumber#getReleaseType() and makes me think you need ReleaseNumberTest. Or, flipside--I see no code that actually uses ReleaseType. Should it even exist? http://gwt-code-reviews.appspot.com/72802/diff/1/4 File dev/core/test/com/google/gwt/dev/shell/CheckForUpdatesTest.java (left): http://gwt-code-reviews.appspot.com/72802/diff/1/4#oldcode6 Line 6: public class CheckForUpdatesTest extends TestCase { Seems like you still need at least some kind of test at this level, to make sure that CheckForUpdates is actually using GwtVerison objects properly. It'd be silly for it to duplicate the coverage of GwtVersionTest, but we still at least want to see that isServerVersionNewer works at all. Also, how did you confirm that the old code will see the new style strings as newer? http://gwt-code-reviews.appspot.com/72802/diff/1/4#oldcode40 Line 40: // assertFalse(CheckForUpdates.isServerVersionNewer("2", "1.2.3")); Oh. You're telling me that the tests I'm saying we shouldn't delete have actually never existed? Why is CheckForUpdates#isServerVersionNewer() untestable? http://gwt-code-reviews.appspot.com/72802 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
