Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2648203003:
Add 1.22 features to CHANGELOG. (Closed)
|
Created:
3 years, 11 months ago by Bob Nystrom Modified:
3 years, 10 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd 1.22 features to CHANGELOG.
R=eernst@google.com, kevmoo@google.com, lrn@google.com, mit@google.com
Committed: https://github.com/dart-lang/sdk/commit/c3f1212a9c5b1105a604b575c9999d7a29ab1d3d
Patch Set 1 #
Total comments: 22
Patch Set 2 : Revise. #
Total comments: 4
Patch Set 3 : Typo. #Patch Set 4 : Tweak generalized tear-offs. #Messages
Total messages: 21 (5 generated)
rnystrom@google.com changed reviewers: + leafp@google.com, mit@google.com
kevmoo@google.com changed reviewers: + kevmoo@google.com
lgtm
lrn@google.com changed reviewers: + lrn@google.com
https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode47 CHANGELOG.md:47: * Introduce `covariant` modifier on parameters. Adding that indicates that the Please say very early that this is only intended for strong-mode, and that spec mode ignores it completely. Same for the other entries. It's somewhat confusing to read this as a primarily spec-mode programmer and only figure out at the end that this is for strong mode, and it doesn't affect spec mode at all - and it doesn't even say that explicitly. Maybe have a sub-section of the ###Language section for strong mode, so it's obvious what it's about. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode138 CHANGELOG.md:138: ``` Please, please, please do not use this example. This is exactly code that we do NOT want use people to write. There should (and hopefully will) be a style guide entry saying to *not* use FutureOr as an actual return type of a method. (I know that this isn't an actual method, since it's abstract, and subclasses can override it to be either async or not, but it still exposes an async-or-not API to arbitrary users, it's not just something itself has to handle) The place where it makes sense is for return types of *parameter* functions: Future<S> then<S>(FutureOr<S> action(T value), ...) Here only the function itself needs to be able to handle the async-or-not result. We use it here because the union type on the return type extends naturally to a union type on the function type, and that is what we really want: Future<S> then<S>((S Function(T) | Future<S> Function(T)) action, ...) We don't actually want "API function" (something exposed to others) where you don't know the return type that other people have to handle. That's bad usability design. It means that you don't know if you need to await the result or not, and you either always await, or you have to do an is-check on the result before using it. It's more usable design to *always* return a Future. (Even methods returning null or a Future have turned out to be annoying in practice, likewise for functions that can throw both synchronously and asynchronously - if it's async, keep it all async). https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode143 CHANGELOG.md:143: of this.) What is the return type of this method? Again, not a choice that I recommend. I'd probably prefer to just pass a function instead of an object. You might have a case where it makes more sense (like knowing that you are the only consumer of transformer objects), but don't encourage it in general.
https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode96 CHANGELOG.md:96: * Change instantiate-to-bounds rules for generic type parameters. If you leave "for generic type parameters when running in strong mode." https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode138 CHANGELOG.md:138: ``` On 2017/01/24 06:52:40, Lasse Reichstein Nielsen wrote: > Please, please, please do not use this example. This is exactly code that we do > NOT want use people to write. > There should (and hopefully will) be a style guide entry saying to *not* use > FutureOr as an actual return type of a method. > (I know that this isn't an actual method, since it's abstract, and subclasses > can override it to be either async or not, but it still exposes an async-or-not > API to arbitrary users, it's not just something itself has to handle) > > The place where it makes sense is for return types of *parameter* functions: > Future<S> then<S>(FutureOr<S> action(T value), ...) > Here only the function itself needs to be able to handle the async-or-not > result. > We use it here because the union type on the return type extends naturally to a > union type on the function type, and that is what we really want: > Future<S> then<S>((S Function(T) | Future<S> Function(T)) action, ...) > > We don't actually want "API function" (something exposed to others) where you > don't know the return type that other people have to handle. That's bad > usability design. It means that you don't know if you need to await the result > or not, and you either always await, or you have to do an is-check on the result > before using it. It's more usable design to *always* return a Future. > (Even methods returning null or a Future have turned out to be annoying in > practice, likewise for functions that can throw both synchronously and > asynchronously - if it's async, keep it all async). +1 Future.value, Future.then are both good examples.
eernst@google.com changed reviewers: + eernst@google.com
DBC: A few comments added. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode33 CHANGELOG.md:33: it is considered a subtype of every other type. We could add the following sentence here, to address the 'WAT?! - wasn't it so already?' experience that readers might otherwise have: 'The `null` value itself has always had such a type, but the following examples used to fail and they will now work:`. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode35 CHANGELOG.md:35: Examples: And then we might as well delete this line. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode47 CHANGELOG.md:47: * Introduce `covariant` modifier on parameters. Adding that indicates that the +1! So at this point we could have 'Support the `covariant` modifier syntax. This modifier has no effect outside strong mode (see the description below).', and then the current text in this bullet goes to a separate strong mode section. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode67 CHANGELOG.md:67: // Eaten by orcas. Ought to add 'class Orca extends Predator ..', and then I guess the comment is unnecessary. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode94 CHANGELOG.md:94: opt into in the places where you do want it using this modifier. 'opt into [it]..', or maybe just 'it lets you use it in the places'? https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode96 CHANGELOG.md:96: * Change instantiate-to-bounds rules for generic type parameters. If you leave On 2017/01/24 07:55:27, Leaf wrote: > "for generic type parameters when running in strong mode." With a separate section for strong mode, this should be located in that section (and the words 'when running in strong mode' could then be omitted). https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode105 CHANGELOG.md:105: var a = new Abser(); // A<dynamic>. '// Abser<dynamic>', presumably.
lgtm https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode47 CHANGELOG.md:47: * Introduce `covariant` modifier on parameters. Adding that indicates that the On 2017/01/24 06:52:40, Lasse Reichstein Nielsen wrote: > Please say very early that this is only intended for strong-mode, and that spec > mode ignores it completely. Same for the other entries. > > It's somewhat confusing to read this as a primarily spec-mode programmer and > only figure out at the end that this is for strong mode, and it doesn't affect > spec mode at all - and it doesn't even say that explicitly. > > Maybe have a sub-section of the ###Language section for strong mode, so it's > obvious what it's about. +1 for having a `### Language (strong mode only)` section
Thanks for all the suggestions! https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode33 CHANGELOG.md:33: it is considered a subtype of every other type. On 2017/01/24 09:13:50, eernst wrote: > We could add the following sentence here, to address the 'WAT?! - wasn't it so > already?' experience that readers might otherwise have: 'The `null` value itself > has always had such a type, but the following examples used to fail and they > will now work:`. Done. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode35 CHANGELOG.md:35: Examples: On 2017/01/24 09:13:51, eernst wrote: > And then we might as well delete this line. Done. Also came up with a more motivating example. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode47 CHANGELOG.md:47: * Introduce `covariant` modifier on parameters. Adding that indicates that the On 2017/01/24 06:52:40, Lasse Reichstein Nielsen wrote: > Please say very early that this is only intended for strong-mode, and that spec > mode ignores it completely. Same for the other entries. > > It's somewhat confusing to read this as a primarily spec-mode programmer and > only figure out at the end that this is for strong mode, and it doesn't affect > spec mode at all - and it doesn't even say that explicitly. > > Maybe have a sub-section of the ###Language section for strong mode, so it's > obvious what it's about. That would be tricky because then we'd still have to mention the syntax in spec mode. Clarified that it only affects strong mode. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode67 CHANGELOG.md:67: // Eaten by orcas. On 2017/01/24 09:13:51, eernst wrote: > Ought to add 'class Orca extends Predator ..', and then I guess the comment is > unnecessary. Done. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode94 CHANGELOG.md:94: opt into in the places where you do want it using this modifier. On 2017/01/24 09:13:51, eernst wrote: > 'opt into [it]..', or maybe just 'it lets you use it in the places'? Done. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode96 CHANGELOG.md:96: * Change instantiate-to-bounds rules for generic type parameters. If you leave On 2017/01/24 07:55:27, Leaf wrote: > "for generic type parameters when running in strong mode." Done. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode105 CHANGELOG.md:105: var a = new Abser(); // A<dynamic>. On 2017/01/24 09:13:51, eernst wrote: > '// Abser<dynamic>', presumably. Done. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode138 CHANGELOG.md:138: ``` On 2017/01/24 06:52:40, Lasse Reichstein Nielsen wrote: > Please, please, please do not use this example. This is exactly code that we do > NOT want use people to write. > There should (and hopefully will) be a style guide entry saying to *not* use > FutureOr as an actual return type of a method. > (I know that this isn't an actual method, since it's abstract, and subclasses > can override it to be either async or not, but it still exposes an async-or-not > API to arbitrary users, it's not just something itself has to handle) > > The place where it makes sense is for return types of *parameter* functions: > Future<S> then<S>(FutureOr<S> action(T value), ...) > Here only the function itself needs to be able to handle the async-or-not > result. > We use it here because the union type on the return type extends naturally to a > union type on the function type, and that is what we really want: > Future<S> then<S>((S Function(T) | Future<S> Function(T)) action, ...) > > We don't actually want "API function" (something exposed to others) where you > don't know the return type that other people have to handle. That's bad > usability design. It means that you don't know if you need to await the result > or not, and you either always await, or you have to do an is-check on the result > before using it. It's more usable design to *always* return a Future. > (Even methods returning null or a Future have turned out to be annoying in > practice, likewise for functions that can throw both synchronously and > asynchronously - if it's async, keep it all async). Came up with a different example. I do think the original one is reasonable too. > (I know that this isn't an actual method, since it's abstract, and subclasses > can override it to be either async or not Right. > it still exposes an async-or-not API to arbitrary users, it's not just something itself has to handle) Not necessarily. Some interfaces are meant to be implemented but not used. The example here is basically a Barback transformers. Users implement the Transformer API, but they never invoke it themselves. Barback does. Barback is happy to handle the async-or-not aspect of it. https://codereview.chromium.org/2648203003/diff/1/CHANGELOG.md#newcode143 CHANGELOG.md:143: of this.) What is the return type of this method? On 2017/01/24 06:52:40, Lasse Reichstein Nielsen wrote: > Again, not a choice that I recommend. I'd probably prefer to just pass a > function instead of an object. You might have a case where it makes more sense > (like knowing that you are the only consumer of transformer objects), but don't > encourage it in general. Yup, fair point. Came up with a better example.
A couple of points https://codereview.chromium.org/2648203003/diff/20001/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/2648203003/diff/20001/CHANGELOG.md#newcode8 CHANGELOG.md:8: de-support them. They were previously supported in the VM only. Would it be possible to add a short alternative to generalized tear-offs? Something like "As an alternative to generalized tear-offs, we recommend you use static methods". Without this, the change may look much worse than it actually is. https://codereview.chromium.org/2648203003/diff/20001/CHANGELOG.md#newcode166 CHANGELOG.md:166: your API strictly synchronous, but then users of simple synchronous "You could make your API strictly synchronous" -- don't you mean asynchronous?
https://codereview.chromium.org/2648203003/diff/20001/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/2648203003/diff/20001/CHANGELOG.md#newcode8 CHANGELOG.md:8: de-support them. They were previously supported in the VM only. On 2017/01/26 21:37:51, filiph wrote: > Would it be possible to add a short alternative to generalized tear-offs? > Something like "As an alternative to generalized tear-offs, we recommend you use > static methods". Without this, the change may look much worse than it actually > is. I don't think it's worth doing. Almost no one on Earth even knows what generalized tear-offs were. https://codereview.chromium.org/2648203003/diff/20001/CHANGELOG.md#newcode166 CHANGELOG.md:166: your API strictly synchronous, but then users of simple synchronous On 2017/01/26 21:37:51, filiph wrote: > "You could make your API strictly synchronous" -- don't you mean asynchronous? Good catch! Done.
> > Would it be possible to add a short alternative to generalized tear-offs? > > Something like "As an alternative to generalized tear-offs, we recommend you use > > static methods". Without this, the change may look much worse than it actually > > is. > > I don't think it's worth doing. Almost no one on Earth even knows what generalized tear-offs were. That's actually why I'm suggesting this. Since we're (correctly) calling it a breaking change, we should somehow make sure people who never knew about generalized tear-offs understand this is not affecting them. My example blurb above is probably not that great. Here's another stab at it: "... They were previously supported in the VM only and their usage was minimal."
On 2017/01/26 22:29:17, filiph wrote: > > > Would it be possible to add a short alternative to generalized tear-offs? > > > Something like "As an alternative to generalized tear-offs, we recommend you > use > > > static methods". Without this, the change may look much worse than it > actually > > > is. > > > > I don't think it's worth doing. Almost no one on Earth even knows what > generalized tear-offs were. > > That's actually why I'm suggesting this. Since we're (correctly) calling it a > breaking change, > we should somehow make sure people who never knew about generalized tear-offs > understand this is not > affecting them. My example blurb above is probably not that great. > > Here's another stab at it: > "... They were previously supported in the VM only and their usage was minimal." Done.
On 2017/01/26 22:42:04, Bob Nystrom wrote: Can this be landed?
LGTM --- I'm not going into the discussion that Lasse started about swizzle, but the stuff I commented on looks fine!
lgtm
Description was changed from ========== Add 1.22 features to CHANGELOG. ========== to ========== Add 1.22 features to CHANGELOG. R=eernst@google.com, kevmoo@google.com, lrn@google.com, mit@google.com Committed: https://github.com/dart-lang/sdk/commit/c3f1212a9c5b1105a604b575c9999d7a29ab1d3d ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as c3f1212a9c5b1105a604b575c9999d7a29ab1d3d (presubmit successful). |
