Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23007017:
Merge branch 'master' into datetimedocs (Closed)
DescriptionMerge branch 'master' into datetimedocs
Top- and member-level docs for DateTime class
BUG=
R=kathyw@google.com, sethladd@google.com
Committed: https://code.google.com/p/dart/source/detail?r=26472
Patch Set 1 #
Total comments: 67
Patch Set 2 : Integrated KWs most excellent comments. #
Total comments: 26
Patch Set 3 : incorporated KW and Seth's review comments #
Total comments: 2
Patch Set 4 : removed fictional phrase #Patch Set 5 : merge with master #Messages
Total messages: 11 (0 generated)
this one has fuller docs and code snippets.
this one has fuller docs and code snippets
bunch of nits https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart File sdk/lib/core/date_time.dart (right): https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:11: * See [Stopwatch] to measure time-spans. timespan is one word (I checked http://dictionary.reference.com/browse/timespan and googled [define timespan]) https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:15: * For example, , -> : https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:21: * Once created, a DateTime object cannot be changed; It has no setters. It -> it https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:23: * Use getters to access How about just: You can use properties to get the individual... (Properties is what fields/ivars are called in the TOC.) https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:29: * the DateTime class provides a constant for each day and month name, name, -> name— https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:30: * `AUGUST` and `FRIDAY` for example. "for example" is parenthetical and should be set off by a comma. So it should be either: * `AUGUST` and `FRIDAY`, for example. * for example, `AUGUST` and `FRIDAY`. (I prefer the latter.) https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:31: * You can use these with the constructors these -> these constants the constructors -> constructors https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:32: * or other places in your code where needed: "where needed" is unhelpful and seems formal. Maybe just drop everything after "constructors". https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:37: * Day and month values begin at 1 and the week starts on Monday. and -> , and https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:50: * Use the methods `toLocal()` and `toUtc()` to convert between UTC and local time. line length > 80 chars? (this happens a bunch of times) https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:53: * and to find out the difference between UTC and the time zone of a DateTime object Split this into its own sentence. ` and to -> `. To https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:59: * such as `isAfter()`, `isBefore()`, and `isAtSameMomentAs()`, should these be [blah] instead, so they'll be links? (globally) https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:67: * Use the `add()` and `subtract()` methods in conjunction with a [Duration] object in conjunction with -> with https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:85: * Use the [intl](http://pub.dartlang.org/packages/intl) package Use -> To internationalize your code, use https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:86: * at the pub shared packages repo. delete this line? https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:131: * assert(dDay.isUtc()); remove () https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:148: * The value of [hour] is between 0 and 23, as in a 24-hour clock. This duplicates info that's already in the class description: * moon landing example * one-based * discussion of declared constants https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:171: * Coordinated Universal Time. -> UTC https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:174: * The value of [hour] is between 0 and 23, as in a 24-hour clock. Could this be pulled out into the class description? https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:295: * Returns true if [this] occurs before [other]. The comparison is independent Make the first paragraph one sentence long. I don't know what "is independent of" means. Does that mean it takes the time zone into account, or not? https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:309: * Returns true if [this] occurs after [other]. The comparison is independent See comment for isBefore(). https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:323: * Returns true if [this] occurs at the same moment as [other]. The See comment for isBefore(). https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:338: * Returns: Should be in a separate paragraph. I'm not crazy about the construction of this, especially the punctuation at the ends of each bulleted item. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:351: * Returns [this] if it is already in the local time zone. convert this first paragraph into a single sentence https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:366: * Returns [this] if it is already in UTC. convert this first paragraph into a single sentence https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:379: * Returns a human-readable string for this instance. add a blank line after this one. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:476: * Returns the abbreviated time-zone name. For example: [:"CET":] or [:"CEST":]. convert this first paragraph into a single sentence https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:499: * Returns the month into the year [1..12]. into -> of https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:508: * Returns the day into the month [1..31]. into -> of https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:516: * Returns the hour into the day [0..23]. into -> of https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:524: * Returns the minute into the hour [0...59]. into -> of https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:532: * Returns the second into the minute [0...59]. into -> of https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:540: * Returns the millisecond into the second [0...999]. into -> of? After a while, "into/of the xyz" just starts to look weird to me. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:548: * Returns the week day [MON..SUN]. In accordance with ISO 8601 week day -> day of the week Put a blank line before "In". which -> , which
Not sure what to do about the "independent of time zone" bizness. mem https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart File sdk/lib/core/date_time.dart (right): https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:11: * See [Stopwatch] to measure time-spans. On 2013/08/19 22:06:21, Kathy Walrath wrote: > timespan is one word > > (I checked http://dictionary.reference.com/browse/timespan and googled [define > timespan]) Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:15: * For example, On 2013/08/19 22:06:21, Kathy Walrath wrote: > , -> : Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:21: * Once created, a DateTime object cannot be changed; It has no setters. On 2013/08/19 22:06:21, Kathy Walrath wrote: > It -> it Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:23: * Use getters to access On 2013/08/19 22:06:21, Kathy Walrath wrote: > How about just: > > You can use properties to get the individual... > > (Properties is what fields/ivars are called in the TOC.) Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:29: * the DateTime class provides a constant for each day and month name, On 2013/08/19 22:06:21, Kathy Walrath wrote: > name, -> name— Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:30: * `AUGUST` and `FRIDAY` for example. On 2013/08/19 22:06:21, Kathy Walrath wrote: > "for example" is parenthetical and should be set off by a comma. So it should be > either: > > * `AUGUST` and `FRIDAY`, for example. > * for example, `AUGUST` and `FRIDAY`. > > (I prefer the latter.) Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:31: * You can use these with the constructors On 2013/08/19 22:06:21, Kathy Walrath wrote: > these -> these constants > the constructors -> constructors Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:32: * or other places in your code where needed: On 2013/08/19 22:06:21, Kathy Walrath wrote: > "where needed" is unhelpful and seems formal. Maybe just drop everything after > "constructors". Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:32: * or other places in your code where needed: On 2013/08/19 22:06:21, Kathy Walrath wrote: > "where needed" is unhelpful and seems formal. Maybe just drop everything after > "constructors". Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:37: * Day and month values begin at 1 and the week starts on Monday. On 2013/08/19 22:06:21, Kathy Walrath wrote: > and -> , and Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:50: * Use the methods `toLocal()` and `toUtc()` to convert between UTC and local time. On 2013/08/19 22:06:21, Kathy Walrath wrote: > line length > 80 chars? > > (this happens a bunch of times) Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:53: * and to find out the difference between UTC and the time zone of a DateTime object On 2013/08/19 22:06:21, Kathy Walrath wrote: > Split this into its own sentence. > > ` and to -> `. To Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:59: * such as `isAfter()`, `isBefore()`, and `isAtSameMomentAs()`, On 2013/08/19 22:06:21, Kathy Walrath wrote: > should these be [blah] instead, so they'll be links? > > (globally) Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:67: * Use the `add()` and `subtract()` methods in conjunction with a [Duration] object On 2013/08/19 22:06:21, Kathy Walrath wrote: > in conjunction with -> with Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:85: * Use the [intl](http://pub.dartlang.org/packages/intl) package On 2013/08/19 22:06:21, Kathy Walrath wrote: > Use -> To internationalize your code, use Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:86: * at the pub shared packages repo. On 2013/08/19 22:06:21, Kathy Walrath wrote: > delete this line? Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:131: * assert(dDay.isUtc()); On 2013/08/19 22:06:21, Kathy Walrath wrote: > remove () Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:148: * The value of [hour] is between 0 and 23, as in a 24-hour clock. On 2013/08/19 22:06:21, Kathy Walrath wrote: > This duplicates info that's already in the class description: > > * moon landing example > * one-based > * discussion of declared constants Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:171: * Coordinated Universal Time. On 2013/08/19 22:06:21, Kathy Walrath wrote: > -> UTC Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:174: * The value of [hour] is between 0 and 23, as in a 24-hour clock. On 2013/08/19 22:06:21, Kathy Walrath wrote: > Could this be pulled out into the class description? Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:295: * Returns true if [this] occurs before [other]. The comparison is independent I'm leaving this for now. Not sure what to say instead. On 2013/08/19 22:06:21, Kathy Walrath wrote: > Make the first paragraph one sentence long. > > I don't know what "is independent of" means. Does that mean it takes the time > zone into account, or not? https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:338: * Returns: On 2013/08/19 22:06:21, Kathy Walrath wrote: > Should be in a separate paragraph. > I'm not crazy about the construction of this, especially the punctuation at the > ends of each bulleted item. Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:351: * Returns [this] if it is already in the local time zone. On 2013/08/19 22:06:21, Kathy Walrath wrote: > convert this first paragraph into a single sentence Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:366: * Returns [this] if it is already in UTC. On 2013/08/19 22:06:21, Kathy Walrath wrote: > convert this first paragraph into a single sentence Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:379: * Returns a human-readable string for this instance. On 2013/08/19 22:06:21, Kathy Walrath wrote: > add a blank line after this one. Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:476: * Returns the abbreviated time-zone name. For example: [:"CET":] or [:"CEST":]. On 2013/08/19 22:06:21, Kathy Walrath wrote: > convert this first paragraph into a single sentence Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:476: * Returns the abbreviated time-zone name. For example: [:"CET":] or [:"CEST":]. On 2013/08/19 22:06:21, Kathy Walrath wrote: > convert this first paragraph into a single sentence Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:499: * Returns the month into the year [1..12]. On 2013/08/19 22:06:21, Kathy Walrath wrote: > into -> of Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:508: * Returns the day into the month [1..31]. On 2013/08/19 22:06:21, Kathy Walrath wrote: > into -> of Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:516: * Returns the hour into the day [0..23]. On 2013/08/19 22:06:21, Kathy Walrath wrote: > into -> of Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:524: * Returns the minute into the hour [0...59]. On 2013/08/19 22:06:21, Kathy Walrath wrote: > into -> of Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:532: * Returns the second into the minute [0...59]. On 2013/08/19 22:06:21, Kathy Walrath wrote: > into -> of Done. https://codereview.chromium.org/23007017/diff/1/sdk/lib/core/date_time.dart#n... sdk/lib/core/date_time.dart:548: * Returns the week day [MON..SUN]. In accordance with ISO 8601 On 2013/08/19 22:06:21, Kathy Walrath wrote: > week day -> day of the week > > Put a blank line before "In". > > which -> , which Done.
https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dart File sdk/lib/core/date_time.dart (right): https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:8: * An instant in time, such as July 20, 1969, 8:18pm. I'm left wondering about timezone here. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:11: * or by parsing a correctly formatted string. where is the definition of a correctly formatted string? https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:20: * Once created, a DateTime object cannot be changed; it has no setters. I don't think we need "it has no setters" https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:41: * The DateTime class provides support for handling time zones. I think this is too generous. This only creates dates in either UTC or local time zone. "handling time zones" implies I can create a date with any time zone, or adjust to a time zone, etc. Let's pull this back and stress this class barely knows about time zones. And if a user was interested in time zones, have them use the XXX class. Which I don't know what that is, so someone please tell me :) https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:49: * Use the methods [toLocal] and [toUtc] I'm left wondering how DateTime knows my local timezone. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:52: * To discover the name of the time zone use [timeZoneName]. can we be more specific? "the time zone used when the datetime was created" https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:88: * the [intl](http://pub.dartlang.org/packages/intl) package. nice link!
1 big question (how to document read-only properties), plus a smaller question and some nits. It's looking good, though! https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dart File sdk/lib/core/date_time.dart (right): https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:118: * Returns the number of milliseconds since Hmmm... This brings up the whole property vs. method thing. To me, it feels like properties should have noun phrases, and that you should mention "read-only" if necessary. (Which it might not be, since you've already said that DateTimes are immutable, though not using that exact word.) If you need to indicate that a field is read-only, maybe use: The blah. (read only) OR The blah. [read only] OR Read only: The blah. OR [read only] The blah. Ew. Maybe you can think of something better. It just seems weird to use a whole different kind of phrase for a read-only property, as opposed to a read-write property. It exposes implementation in a slightly yucky way. But maybe "Returns" (or better: "Gets") is the best solution. How does it read in the docs? PS: I just did a little research. MDN one liners don't necessarily bother telling you that a property's read-only. An example: childElementcount in https://developer.mozilla.org/en-US/docs/Web/API/element. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:131: * Returns true if this [DateTime] is set to UTC time. -> True...? https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:140: * Constructs a [DateTime] instance based on the individual units. Maybe combine the first 2 sentences into 1: Constructs a [DateTime] instance that uses the local time zone. [I'm unclear. Does "local" mean the user's local time zone, at the time the code is run? Or the DateTime is just interpreted relative to whatever the local time zone happens to be at the time the DateTime is used?] https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:163: * The date is in the UTC time zone. Maybe just combine the first 2 sentences into one: Constructs a [DateTime instance] that uses the UTC time zone. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:340: * or a positive integer if it is greater (later). I like the rewrite! https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:351: * otherwise this method is equivalent to: , o -> . O
Et voila. mem https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dart File sdk/lib/core/date_time.dart (right): https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:8: * An instant in time, such as July 20, 1969, 8:18pm. On 2013/08/20 19:32:49, sethladd wrote: > I'm left wondering about timezone here. Done. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:11: * or by parsing a correctly formatted string. On 2013/08/20 19:32:49, sethladd wrote: > where is the definition of a correctly formatted string? Done. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:20: * Once created, a DateTime object cannot be changed; it has no setters. On 2013/08/20 19:32:49, sethladd wrote: > I don't think we need "it has no setters" Done. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:41: * The DateTime class provides support for handling time zones. On 2013/08/20 19:32:49, sethladd wrote: > I think this is too generous. This only creates dates in either UTC or local > time zone. "handling time zones" implies I can create a date with any time zone, > or adjust to a time zone, etc. Let's pull this back and stress this class barely > knows about time zones. And if a user was interested in time zones, have them > use the XXX class. Which I don't know what that is, so someone please tell me :) Done. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:49: * Use the methods [toLocal] and [toUtc] On 2013/08/20 19:32:49, sethladd wrote: > I'm left wondering how DateTime knows my local timezone. Done. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:52: * To discover the name of the time zone use [timeZoneName]. On 2013/08/20 19:32:49, sethladd wrote: > can we be more specific? "the time zone used when the datetime was created" Done. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:88: * the [intl](http://pub.dartlang.org/packages/intl) package. On 2013/08/20 19:32:49, sethladd wrote: > nice link! Done. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:118: * Returns the number of milliseconds since The guidelines DO say that vars, getters should be noun phrases. So I fixed those. The signature does say 'final' so is that enough? mem On 2013/08/20 19:58:23, Kathy Walrath wrote: > Hmmm... This brings up the whole property vs. method thing. > > To me, it feels like properties should have noun phrases, and that you should > mention "read-only" if necessary. (Which it might not be, since you've already > said that DateTimes are immutable, though not using that exact word.) > > If you need to indicate that a field is read-only, maybe use: > > The blah. (read only) > OR > The blah. [read only] > OR > Read only: The blah. > OR > [read only] The blah. > > Ew. Maybe you can think of something better. > > It just seems weird to use a whole different kind of phrase for a read-only > property, as opposed to a read-write property. It exposes implementation in a > slightly yucky way. > > But maybe "Returns" (or better: "Gets") is the best solution. How does it read > in the docs? > > PS: I just did a little research. MDN one liners don't necessarily bother > telling you that a property's read-only. An example: childElementcount in > https://developer.mozilla.org/en-US/docs/Web/API/element. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:131: * Returns true if this [DateTime] is set to UTC time. On 2013/08/20 19:58:23, Kathy Walrath wrote: > -> True...? Done. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:140: * Constructs a [DateTime] instance based on the individual units. On 2013/08/20 19:58:23, Kathy Walrath wrote: > Maybe combine the first 2 sentences into 1: > > Constructs a [DateTime] instance that uses the local time zone. > > [I'm unclear. Does "local" mean the user's local time zone, at the time the code > is run? Or the DateTime is just interpreted relative to whatever the local time > zone happens to be at the time the DateTime is used?] Done. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:163: * The date is in the UTC time zone. On 2013/08/20 19:58:23, Kathy Walrath wrote: > Maybe just combine the first 2 sentences into one: > > Constructs a [DateTime instance] that uses the UTC time zone. Done. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:340: * or a positive integer if it is greater (later). On 2013/08/20 19:58:23, Kathy Walrath wrote: > I like the rewrite! Done. https://codereview.chromium.org/23007017/diff/7001/sdk/lib/core/date_time.dar... sdk/lib/core/date_time.dart:351: * otherwise this method is equivalent to: On 2013/08/20 19:58:23, Kathy Walrath wrote: > , o -> . O Done.
lgtm but I'd really like to see us document the date strings we can parse. https://codereview.chromium.org/23007017/diff/13001/sdk/lib/core/date_time.dart File sdk/lib/core/date_time.dart (right): https://codereview.chromium.org/23007017/diff/13001/sdk/lib/core/date_time.da... sdk/lib/core/date_time.dart:12: * which complies with a subset of ISO 8601. where is this defined?
lgtm
https://codereview.chromium.org/23007017/diff/13001/sdk/lib/core/date_time.dart File sdk/lib/core/date_time.dart (right): https://codereview.chromium.org/23007017/diff/13001/sdk/lib/core/date_time.da... sdk/lib/core/date_time.dart:12: * which complies with a subset of ISO 8601. there is an open bug (1878) about this. ...asking for specificiation for the grammar for these strings. mem On 2013/08/21 20:15:09, sethladd wrote: > where is this defined?
Message was sent while issue was closed.
Committed patchset #5 manually as r26472 (presubmit successful). |
