Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10383203:
Add base Version class to start support versioning in pub. (Closed)
|
Created:
8 years, 7 months ago by Bob Nystrom Modified:
8 years, 7 months ago Reviewers:
nweiz CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd base Version class to start support versioning in pub.
Committed: https://code.google.com/p/dart/source/detail?r=7701
Patch Set 1 #
Total comments: 31
Patch Set 2 : Respond to review. #Patch Set 3 : Fix ==. #
Total comments: 6
Messages
Total messages: 8 (0 generated)
Starting to hack on versioning going bottom-up.
https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart File utils/pub/version.dart (right): https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:10: #library('pub_version'); Why is this a separate library? Are you testing out the post-#source one-library-per-file philosophy? https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:17: @'(-([0-9A-Za-z-.]+(\.[0-9A-Za-z-.]+)*))?' // Pre-release. I believe the way this is currnetly written, the first "[0-9A-Za-z-.]+" will greedily consume all the characters, leaving nothing for the second capture. Maybe it shouldn't include "."? Also, I think ".-" at the end of the [] is clearer than "-.", since it makes it obvious that the hyphen isn't defining a range. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:24: static Version parse(String text) { Factory constructor? https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:42: ensure(patch >= 0); I don't think it's possible for these to be < 0. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:44: return new Version(major, minor, patch, pre: preRelease, build: build); I don't like using explicit keyword arguments when they're just repeating the variable names. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:67: : preRelease = pre { Why not use "this." for pre/preRelease? https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:76: bool operator ==(Version other) { Shouldn't this take an untyped "other" parameter? https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:193: class VersionRange implements VersionConstraint { Won't users potentially want to specify "> min" or "<= max"? It seems like maybe this should just keep flags for whether the min and max are inclusive or exclusive. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... File utils/tests/pub/version_test.dart (right): https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... utils/tests/pub/version_test.dart:128: test('allows omitting max', () { What about "allows omitting min"? https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... utils/tests/pub/version_test.dart:136: var range = new VersionRange(); Why is this allowed? https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... utils/tests/pub/version_test.dart:169: test('has no max if one was not set', () { What about "has no min if one was not set"? https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... utils/tests/pub/version_test.dart:177: test('allows any version if there is no min or max', () { It seems like this might be cleaner to represent as a special AnyVersion constraint.
'Tanks! https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart File utils/pub/version.dart (right): https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:10: #library('pub_version'); On 2012/05/15 23:12:19, nweiz wrote: > Why is this a separate library? Are you testing out the post-#source > one-library-per-file philosophy? Sort of. It doesn't depend on anything in pub, so it seemed nice to pull it out. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:17: @'(-([0-9A-Za-z-.]+(\.[0-9A-Za-z-.]+)*))?' // Pre-release. On 2012/05/15 23:12:19, nweiz wrote: > I believe the way this is currnetly written, the first "[0-9A-Za-z-.]+" will > greedily consume all the characters, leaving nothing for the second capture. > Maybe it shouldn't include "."? Exactly right. Accidentally left that in from before I was separating out the dotted sections. > Also, I think ".-" at the end of the [] is clearer than "-.", since it makes it > obvious that the hyphen isn't defining a range. Done. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:24: static Version parse(String text) { On 2012/05/15 23:12:19, nweiz wrote: > Factory constructor? Done. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:42: ensure(patch >= 0); On 2012/05/15 23:12:19, nweiz wrote: > I don't think it's possible for these to be < 0. Done. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:44: return new Version(major, minor, patch, pre: preRelease, build: build); On 2012/05/15 23:12:19, nweiz wrote: > I don't like using explicit keyword arguments when they're just repeating the > variable names. Done. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:67: : preRelease = pre { On 2012/05/15 23:12:19, nweiz wrote: > Why not use "this." for pre/preRelease? I wanted the named argument to be "pre" for brevity, but not the field. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:76: bool operator ==(Version other) { On 2012/05/15 23:12:19, nweiz wrote: > Shouldn't this take an untyped "other" parameter? It did at first, but then the first line was: if (other is! Version) throw... then I realized that's what checked mode is for. This way, it communicates at the type signature level what this method expects. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:193: class VersionRange implements VersionConstraint { On 2012/05/15 23:12:19, nweiz wrote: > Won't users potentially want to specify "> min" or "<= max"? It seems like maybe > this should just keep flags for whether the min and max are inclusive or > exclusive. Yeah, I thought about that. I know we'll want these semantics, so I figured I'd start with this at first and maybe expand later if we want that too. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... File utils/tests/pub/version_test.dart (right): https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... utils/tests/pub/version_test.dart:136: var range = new VersionRange(); On 2012/05/15 23:12:19, nweiz wrote: > Why is this allowed? My plan is that once we start solving for version constraints, I'll add intersect to VersionRange (and Version). Dependencies where a version constraint isn't specified will just be new VersionRange(); Also, min and max are named and optional, so it would have to do *something* here. This seemed like a consistent semantic. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... utils/tests/pub/version_test.dart:169: test('has no max if one was not set', () { On 2012/05/15 23:12:19, nweiz wrote: > What about "has no min if one was not set"? Done. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... utils/tests/pub/version_test.dart:177: test('allows any version if there is no min or max', () { On 2012/05/15 23:12:19, nweiz wrote: > It seems like this might be cleaner to represent as a special AnyVersion > constraint. min and max are optional args, and open either end of the range when omitted so it made sense to me to allow omitting both and just open the whole range. Once I start doing intersection of VersionConstraints, it will be simpler if I have fewer classes I need to double-dispatch on.
https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart File utils/pub/version.dart (right): https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:10: #library('pub_version'); On 2012/05/16 00:27:27, Bob Nystrom wrote: > On 2012/05/15 23:12:19, nweiz wrote: > > Why is this a separate library? Are you testing out the post-#source > > one-library-per-file philosophy? > > Sort of. It doesn't depend on anything in pub, so it seemed nice to pull it out. Neither does source.dart. I think if we want to go this route, we should commit to removing #source entirely in pub. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:76: bool operator ==(Version other) { On 2012/05/16 00:27:27, Bob Nystrom wrote: > On 2012/05/15 23:12:19, nweiz wrote: > > Shouldn't this take an untyped "other" parameter? > > It did at first, but then the first line was: > > if (other is! Version) throw... > > then I realized that's what checked mode is for. This way, it communicates at > the type signature level what this method expects. The first line should actually be: if (other is! Version) return false For example, "1 == []" doesn't throw an error, it returns false. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... File utils/tests/pub/version_test.dart (right): https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... utils/tests/pub/version_test.dart:177: test('allows any version if there is no min or max', () { On 2012/05/16 00:27:27, Bob Nystrom wrote: > On 2012/05/15 23:12:19, nweiz wrote: > > It seems like this might be cleaner to represent as a special AnyVersion > > constraint. > > min and max are optional args, and open either end of the range when omitted so > it made sense to me to allow omitting both and just open the whole range. > > Once I start doing intersection of VersionConstraints, it will be simpler if I > have fewer classes I need to double-dispatch on. It seems like what you're planning to do with versions has all VersionConstraints providing basically the same semantics as VersionRange does now. Maybe a better architecture would be having all the classes just inherit from VersionRange? Alternately, if you do want the full power of an arbitrary allows() method, maybe intersection() would be better represented as an IntersectedVersion class that just checks v1.allows(...) && v2.allows(...).
Thanks! https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart File utils/pub/version.dart (right): https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:10: #library('pub_version'); On 2012/05/16 18:28:05, nweiz wrote: > On 2012/05/16 00:27:27, Bob Nystrom wrote: > > On 2012/05/15 23:12:19, nweiz wrote: > > > Why is this a separate library? Are you testing out the post-#source > > > one-library-per-file philosophy? > > > > Sort of. It doesn't depend on anything in pub, so it seemed nice to pull it > out. > > Neither does source.dart. I think if we want to go this route, we should commit > to removing #source entirely in pub. I'm OK with that if you are, but I think we should incrementally move the existing code over to that model. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:76: bool operator ==(Version other) { On 2012/05/16 18:28:05, nweiz wrote: > On 2012/05/16 00:27:27, Bob Nystrom wrote: > > On 2012/05/15 23:12:19, nweiz wrote: > > > Shouldn't this take an untyped "other" parameter? > > > > It did at first, but then the first line was: > > > > if (other is! Version) throw... > > > > then I realized that's what checked mode is for. This way, it communicates at > > the type signature level what this method expects. > > The first line should actually be: > > if (other is! Version) return false > > For example, "1 == []" doesn't throw an error, it returns false. Done. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... File utils/tests/pub/version_test.dart (right): https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... utils/tests/pub/version_test.dart:128: test('allows omitting max', () { On 2012/05/15 23:12:19, nweiz wrote: > What about "allows omitting min"? Done. https://chromiumcodereview.appspot.com/10383203/diff/1/utils/tests/pub/versio... utils/tests/pub/version_test.dart:177: test('allows any version if there is no min or max', () { On 2012/05/16 18:28:05, nweiz wrote: > On 2012/05/16 00:27:27, Bob Nystrom wrote: > > On 2012/05/15 23:12:19, nweiz wrote: > > > It seems like this might be cleaner to represent as a special AnyVersion > > > constraint. > > > > min and max are optional args, and open either end of the range when omitted > so > > it made sense to me to allow omitting both and just open the whole range. > > > > Once I start doing intersection of VersionConstraints, it will be simpler if I > > have fewer classes I need to double-dispatch on. > > It seems like what you're planning to do with versions has all > VersionConstraints providing basically the same semantics as VersionRange does > now. Maybe a better architecture would be having all the classes just inherit > from VersionRange? Maybe. I kind of sketched this stuff in just for kicks. Once I start actually using version ranges in later patches, I'll have a clearer idea of what I need here. > > Alternately, if you do want the full power of an arbitrary allows() method, > maybe intersection() would be better represented as an IntersectedVersion class > that just checks v1.allows(...) && v2.allows(...).
lgtm https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart File utils/pub/version.dart (right): https://chromiumcodereview.appspot.com/10383203/diff/1/utils/pub/version.dart... utils/pub/version.dart:10: #library('pub_version'); On 2012/05/16 18:39:52, Bob Nystrom wrote: > On 2012/05/16 18:28:05, nweiz wrote: > > On 2012/05/16 00:27:27, Bob Nystrom wrote: > > > On 2012/05/15 23:12:19, nweiz wrote: > > > > Why is this a separate library? Are you testing out the post-#source > > > > one-library-per-file philosophy? > > > > > > Sort of. It doesn't depend on anything in pub, so it seemed nice to pull it > > out. > > > > Neither does source.dart. I think if we want to go this route, we should > commit > > to removing #source entirely in pub. > > I'm OK with that if you are, but I think we should incrementally move the > existing code over to that model. Sounds good.
https://chromiumcodereview.appspot.com/10383203/diff/10001/utils/pub/version.... File utils/pub/version.dart (right): https://chromiumcodereview.appspot.com/10383203/diff/10001/utils/pub/version.... utils/pub/version.dart:176: interface VersionConstraint { couldn't this just be: typedef bool VersionConstraint(Version version); Then, using the below linked gist, you could get a VersionConstraint with: new Interval<Version>(oneVersion, otherVersion).contains https://chromiumcodereview.appspot.com/10383203/diff/10001/utils/pub/version.... utils/pub/version.dart:186: class VersionRange implements VersionConstraint { There should be a generic interface for intervals of Comparables somewhere somewhere in a core library, see: https://gist.github.com/2719638 https://chromiumcodereview.appspot.com/10383203/diff/10001/utils/pub/version.... utils/pub/version.dart:190: VersionRange([this.min, this.max]) { As in the above linked gist, you can determine the min and max in the constructor, instead of requiring the caller to specify them in the correct order.
Thanks for the feedback. This stuff is still a work-in-progress. Versioning is a big chunk of work and I'm trying to break it into smaller patches. Hopefully some of this stuff will make more sense once more patches are in. https://chromiumcodereview.appspot.com/10383203/diff/10001/utils/pub/version.... File utils/pub/version.dart (right): https://chromiumcodereview.appspot.com/10383203/diff/10001/utils/pub/version.... utils/pub/version.dart:176: interface VersionConstraint { On 2012/05/17 15:35:39, seaneagan1 wrote: > couldn't this just be: > > typedef bool VersionConstraint(Version version); It could be right now, but later patches will be adding methods to this interface. https://chromiumcodereview.appspot.com/10383203/diff/10001/utils/pub/version.... utils/pub/version.dart:186: class VersionRange implements VersionConstraint { On 2012/05/17 15:35:39, seaneagan1 wrote: > There should be a generic interface for intervals of Comparables somewhere > somewhere in a core library, see: > > https://gist.github.com/2719638 I like this idea, but sometimes I think you can be *too* generic. VersionRange will likely have operations in later patches that are more specific to versions and would have trouble cramming that in a generic Interval class. Also, at least right now, making changes to corelib is a slow painful process. https://chromiumcodereview.appspot.com/10383203/diff/10001/utils/pub/version.... utils/pub/version.dart:190: VersionRange([this.min, this.max]) { On 2012/05/17 15:35:39, seaneagan1 wrote: > As in the above linked gist, you can determine the min and max in the > constructor, instead of requiring the caller to specify them in the correct > order. At least in the use case of versions, my hunch is that an out-of-order VersionRange constructor call is a sign of a bug, and I'd like to catch that early. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
