Thanks for the review.
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> { On 2009/10/26 17:01:01, Ray Ryan wrote: > 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. The old implementation allowed arbitrary leading and trailing garbage, and when a recent change broke that we got complaints. If we are going to retain that behavior, it is difficult to differentiate an invalid suffix from trailing garbage that is to be ignored. Likewise, that means we can't just leave whatever garbage follows the version number (where do we stop, the next space? the end of the string?) as the release type. The main thing is adding three types of releases (and soon to be 4, when we finalize what a snapshot build will look like) and sorting them properly for upgrade notifications. http://gwt-code-reviews.appspot.com/72802/diff/2001/3006#newcode157 Line 157: private static final String DEFAULT_NO_NAG_VERSION = "0.0.999"; On 2009/10/26 17:01:01, Ray Ryan wrote: > Seems like all these 999s should be a constant. And create another constructor that takes int[] and ReleaseNumber, or reconstruct a string from the number? http://gwt-code-reviews.appspot.com/72802/diff/2001/3006#newcode169 Line 169: // TODO(jat): make final On 2009/10/26 17:01:01, Ray Ryan wrote: > Why TODO and not now? The problem is that it would have to be set in the constructor rather than the parse method. I wasn't sure which way would be best to solve it. All of them are somewhat ugly, but probably the least bad solution would be to have parse return ReleaseNumber and have the constructor do the assign. If that is acceptable or you have a different solution that would be better, I would be happy to fix it now. http://gwt-code-reviews.appspot.com/72802/diff/2001/3006#newcode251 Line 251: public boolean isReleased() { On 2009/10/26 17:01:01, Ray Ryan wrote: > 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. It was not used before because the old system had no concept of different types of releases. We could delete it, though it seems likely that we will want to use it in the future (such as to avoid suggesting replacing a release version with a milestone, for example). It can always be added then if you would prefer to leave it out now. 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"); On 2009/10/26 17:01:01, Ray Ryan wrote: > Why is it good that these are equal? Just verifying that trailing garbage does not cause a version mismatch. http://gwt-code-reviews.appspot.com/72802 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
