On Fri, Sep 25, 2009 at 4:46 PM, <[email protected]> wrote: > > 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 > On 2009/09/25 23:25:39, Ray Ryan wrote: > >> I take it getGwtVersionArray was not public api? Would library writers >> > be > >> relying on it? >> > > The point is even if they were it cannot represent these version > numbers. Seems better to break them so they use the correct API rather > than silently get incorrect results. > > http://gwt-code-reviews.appspot.com/72802/diff/1/5#newcode97 > Line 97: gwtVersionNum = tmp; > On 2009/09/25 23:25:39, Ray Ryan wrote: > >> Seems like gwtVersionNum field should go away, and String >> > getGwtVersionNum() > >> should now be gwtVersion().getNumberString(), something like that. >> > > That would be more of a breaking change. > > 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 it isn't just a GwtVersion, but a ProductInfo or something similar. > I'm ok with that, but that seems like feature creep. > > 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#newcode135 > Line 135: public enum ReleaseType { > On 2009/09/25 23:25:39, Ray Ryan wrote: > >> why public >> > > If GwtVersion is the public API for getting a structured version number, > you need to be able to make sense of its components.
> > 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. > On 2009/09/25 23:25:39, Ray Ryan wrote: > >> Why? >> > > For proper sorting, so 2 comes before 12. > > http://gwt-code-reviews.appspot.com/72802/diff/1/7#newcode401 > Line 401: public List<VersionComponent> getComponents() { > On 2009/09/25 23:25:39, Ray Ryan wrote: > >> I don't see any calls to this. If I didn't miss some, why is it >> > public? Why does > >> it exist? >> > > As above, if this is to be the recommendation for how external code > deals with GWT versions, it needs to be usable as such. To me this is the fundamental question: how did we get from nicer version strings to a public api for dealing with GWT versions? Who is asking for this new feature? Anyway, I think Andrew has made the call that this is not an MS1 issue, so this can go back on ice for now. > > > 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() { > On 2009/09/25 23:25:39, Ray Ryan wrote: > >> bad test name. This isn't running. >> > > Good catch. > > 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. > On 2009/09/25 23:25:39, Ray Ryan wrote: > >> s/compareTo/equals/ >> > > This is why I think javadoc on test methods is stupid. But I digress. >> > > Ok. > > http://gwt-code-reviews.appspot.com/72802/diff/1/2#newcode197 > Line 197: assertTrue(components.get(1).isInteger()); > On 2009/09/25 23:25:39, Ray Ryan wrote: > >> 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. >> > > Ok. > > 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 { > On 2009/09/25 23:25:39, Ray Ryan wrote: > >> 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? >> > > It could be done by creating a fake server, but it is complicated. > Since we didn't have it currently, it seems separate from this change. > > I'm not sure how to test old code short of copying it into the new code, > but it just grabs exactly three integers and strips leading/trailing > garbage. I think that shouldn't be a problem since any of the new > version numbers should be newer than the old numbers, but in any case > there isn't anything we can do about the old code at this point. > > http://gwt-code-reviews.appspot.com/72802/diff/1/4#oldcode40 > Line 40: // assertFalse(CheckForUpdates.isServerVersionNewer("2", > "1.2.3")); > On 2009/09/25 23:25:39, Ray Ryan wrote: > >> Oh. You're telling me that the tests I'm saying we shouldn't delete >> > have > >> actually never existed? Why is CheckForUpdates#isServerVersionNewer() >> untestable? >> > > isServerVersionNewer was simply a comparator between version numbers, > and was obviated by the tests on GwtVersion. > > > http://gwt-code-reviews.appspot.com/72802 > --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
