Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1247203002:
Allow using analyzer 0.26.0-alpha.0. (Closed)
|
Created:
5 years, 5 months ago by Bob Nystrom Modified:
5 years, 5 months ago CC:
reviews_dartlang.org Base URL:
https://github.com/dart-lang/test.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAllow using analyzer 0.26.0-alpha.0.
R=nweiz@google.com
Committed: https://github.com/dart-lang/test/commit/6f9acac18cf7044e2caafa0a7d59797c570f7b1e
Patch Set 1 #Patch Set 2 : Bump test version. #
Total comments: 1
Messages
Total messages: 10 (2 generated)
rnystrom@google.com changed reviewers: + nweiz@google.com
pquitslund@google.com changed reviewers: + pquitslund@google.com
https://codereview.chromium.org/1247203002/diff/20001/pubspec.yaml File pubspec.yaml (right): https://codereview.chromium.org/1247203002/diff/20001/pubspec.yaml#newcode9 pubspec.yaml:9: analyzer: '>=0.23.0 <=0.26.0-alpha.0' This alpha is likely to get bumped real soon. Can we make this a looser constraint?
On 2015/07/22 00:01:01, pquitslund wrote: > https://codereview.chromium.org/1247203002/diff/20001/pubspec.yaml > File pubspec.yaml (right): > > https://codereview.chromium.org/1247203002/diff/20001/pubspec.yaml#newcode9 > pubspec.yaml:9: analyzer: '>=0.23.0 <=0.26.0-alpha.0' > This alpha is likely to get bumped real soon. Can we make this a looser > constraint? Looser to what? What compatibility promises are you giving for these later versions? - bob
On 2015/07/22 00:02:58, Bob Nystrom wrote: > On 2015/07/22 00:01:01, pquitslund wrote: > > https://codereview.chromium.org/1247203002/diff/20001/pubspec.yaml > > File pubspec.yaml (right): > > > > https://codereview.chromium.org/1247203002/diff/20001/pubspec.yaml#newcode9 > > pubspec.yaml:9: analyzer: '>=0.23.0 <=0.26.0-alpha.0' > > This alpha is likely to get bumped real soon. Can we make this a looser > > constraint? > > Looser to what? What compatibility promises are you giving for these later > versions? > > - bob Great question. We bumped to 0.26 because we made an API breaking change. We marked it alpha because we're still experimenting. To be fair these experiments should not affect test (or dart_style) but we still need to be able to tag them. Should we be doing this differently? Sorry BTW for not having thought through the impact of bumping this version. It seems like I'm not quite done learning this lesson!
On 2015/07/22 00:11:52, pquitslund wrote: > On 2015/07/22 00:02:58, Bob Nystrom wrote: > > On 2015/07/22 00:01:01, pquitslund wrote: > > > https://codereview.chromium.org/1247203002/diff/20001/pubspec.yaml > > > File pubspec.yaml (right): > > > > > > https://codereview.chromium.org/1247203002/diff/20001/pubspec.yaml#newcode9 > > > pubspec.yaml:9: analyzer: '>=0.23.0 <=0.26.0-alpha.0' > > > This alpha is likely to get bumped real soon. Can we make this a looser > > > constraint? > > > > Looser to what? What compatibility promises are you giving for these later > > versions? > > > > - bob > > Great question. We bumped to 0.26 because we made an API breaking change. That's groovy. > We marked it alpha because we're still experimenting. That's fine, but it implies 0.26.0-alpha.1 (et. al) may not be compatible with 0.26.0-alpha.0, which means we can't widen the constraint here to include it until *after* it's released and we test against it. > To be fair these experiments should not affect test (or dart_style) but we still need to be able to tag them. > > Should we be doing this differently? Was your intent to put this change in the actual Dart SDK or just check it into the repo because that's where analyzer is developed? Assuming the latter, this is another case where it would be really handy if analyzer was in a separate GitHub repo so you folks can commit to your heart's content without dragging everything else in the repo that uses analyzer onto it. For example, I can make lots of commits to dart_style and I don't care if they break analysis_server at all. I only need to worry about that when I'm ready to bring a new version of dart_style into the SDK. By checking in directly into the repo, you basically *can't* do experimental changes. If it's in the repo, everything else in the repo needs to be able to rely on it, and it needs to be able to ship the next time a dev build of the SDK is cut. > Sorry BTW for not having thought through the impact of bumping this version. It > seems like I'm not quite done learning this lesson! Dependencies are hard. :( - bob
Bob: lgtm On 2015/07/22 00:11:52, pquitslund wrote: > On 2015/07/22 00:02:58, Bob Nystrom wrote: > > On 2015/07/22 00:01:01, pquitslund wrote: > > > https://codereview.chromium.org/1247203002/diff/20001/pubspec.yaml > > > File pubspec.yaml (right): > > > > > > https://codereview.chromium.org/1247203002/diff/20001/pubspec.yaml#newcode9 > > > pubspec.yaml:9: analyzer: '>=0.23.0 <=0.26.0-alpha.0' > > > This alpha is likely to get bumped real soon. Can we make this a looser > > > constraint? > > > > Looser to what? What compatibility promises are you giving for these later > > versions? > > > > - bob > > Great question. We bumped to 0.26 because we made an API breaking change. We > marked it alpha because we're still experimenting. To be fair these experiments > should not affect test (or dart_style) but we still need to be able to tag them. > > Should we be doing this differently? > > Sorry BTW for not having thought through the impact of bumping this version. It > seems like I'm not quite done learning this lesson! The fundamental issue is that breaking changes are really problematic, and you should strive not to make them. This is exponentially more true for packages like analyzer that are in the Dart repo, since everything there has to be mutually compatible. Looking at the changelog, it seems like the only change in 0.26.0-alpha.0 is adding an optional parameter to UriResolver. This doesn't seem like a breaking change at all, unless you expect external users to implement their own UriResolvers. Even if you do, there would be ways to make this change without it being breaking. You could add a new interface (e.g. UriResolverWithActual) that defined the method with the new parameter, and have clients check for that interface before passing the parameter. That's more painful, but it localizes the pain to your package rather than spreading it across the ecosystem. On a broader scope, you would be able to do more refactors without risking causing users problems if the analyzer package had a well-defined API. Right now almost none of its functionality is exposed publicly (that is, importable without diving into src/). But people need access to that functionality, so by necessity they import private files, which means that almost any refactor could be breaking. If you carefully curated a set of public APIs with strong stability guarantees, you'd have more freedom to change their private underpinnings without breaking users.
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 6f9acac18cf7044e2caafa0a7d59797c570f7b1e (presubmit successful).
Message was sent while issue was closed.
On 2015/07/22 00:36:56, nweiz wrote: > Bob: lgtm > > On 2015/07/22 00:11:52, pquitslund wrote: > > On 2015/07/22 00:02:58, Bob Nystrom wrote: > > > On 2015/07/22 00:01:01, pquitslund wrote: > > > > https://codereview.chromium.org/1247203002/diff/20001/pubspec.yaml > > > > File pubspec.yaml (right): > > > > > > > > > https://codereview.chromium.org/1247203002/diff/20001/pubspec.yaml#newcode9 > > > > pubspec.yaml:9: analyzer: '>=0.23.0 <=0.26.0-alpha.0' > > > > This alpha is likely to get bumped real soon. Can we make this a looser > > > > constraint? > > > > > > Looser to what? What compatibility promises are you giving for these later > > > versions? > > > > > > - bob > > > > Great question. We bumped to 0.26 because we made an API breaking change. We > > marked it alpha because we're still experimenting. To be fair these > experiments > > should not affect test (or dart_style) but we still need to be able to tag > them. > > > > Should we be doing this differently? > > > > Sorry BTW for not having thought through the impact of bumping this version. > It > > seems like I'm not quite done learning this lesson! > > The fundamental issue is that breaking changes are really problematic, and you > should strive not to make them. This is exponentially more true for packages > like analyzer that are in the Dart repo, since everything there has to be > mutually compatible. > > Looking at the changelog, it seems like the only change in 0.26.0-alpha.0 is > adding an optional parameter to UriResolver. This doesn't seem like a breaking > change at all, unless you expect external users to implement their own > UriResolvers. > > Even if you do, there would be ways to make this change without it being > breaking. You could add a new interface (e.g. UriResolverWithActual) that > defined the method with the new parameter, and have clients check for that > interface before passing the parameter. That's more painful, but it localizes > the pain to your package rather than spreading it across the ecosystem. > > On a broader scope, you would be able to do more refactors without risking > causing users problems if the analyzer package had a well-defined API. Right now > almost none of its functionality is exposed publicly (that is, importable > without diving into src/). But people need access to that functionality, so by > necessity they import private files, which means that almost any refactor could > be breaking. If you carefully curated a set of public APIs with strong stability > guarantees, you'd have more freedom to change their private underpinnings > without breaking users. All great points. We've been here before too. :/ I agree that this is just more motivation to tighten up our notion of explicit API (reigning in the implicit bits that have leaked out). Thanks, as always, for the input. |
