Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23173002:
Added fixnum package docs. (Closed)
|
Created:
7 years, 4 months ago by Chris Bracken Modified:
7 years, 4 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdded fixnum package docs.
Original bug:
http://dartbug.com/7834
BUG=
R=kathyw@google.com
Committed: https://code.google.com/p/dart/source/detail?r=26134
Patch Set 1 : Added note on DartVM/dart2js compatibility #
Total comments: 37
Patch Set 2 : Additions based on review comments. #
Total comments: 8
Patch Set 3 : README/package doc de-duplication + fixes #
Total comments: 2
Patch Set 4 : Added pub link to package docs. #
Messages
Total messages: 10 (0 generated)
https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart File pkg/fixnum/lib/fixnum.dart (right): https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart... pkg/fixnum/lib/fixnum.dart:6: * This library provides fixed-size integer support. mention that it contains a 32 bit and 64 signed int? https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart... pkg/fixnum/lib/fixnum.dart:11: * ## Installing ## Do most libraries include pub install directions in the library docs? I would think pub instructions should be left at the package level. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.dart File pkg/fixnum/lib/src/intx.dart (right): https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:12: /** Addition operator. */ Q for Kathy I guess: Should we doc the operators if they do the unsurprising thing? It seems like documenting an implementation of a method defined in another class. In Java we don't do that unless there's specific behavior that differs from the declaration.
Great doc comments. My feedback is of the nitpicky variety. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart File pkg/fixnum/lib/fixnum.dart (right): https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart... pkg/fixnum/lib/fixnum.dart:6: * This library provides fixed-size integer support. Our convention is for the first line to *not* be a complete sentence. How about just: Fixed-size integer support. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart... pkg/fixnum/lib/fixnum.dart:11: * ## Installing ## Why is there a ## after this header? https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart... pkg/fixnum/lib/fixnum.dart:13: * Use [pub][] to install this package. Add the following to your `pubspec.yaml` I'm not used to this style of linking. Is there a reason why you used this style here and the []() style later? I'm inclined to use the []() style. Maybe I should put this in the style guidelines unless there's a good reason not to. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart... pkg/fixnum/lib/fixnum.dart:14: * file. file. -> file: https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.dart File pkg/fixnum/lib/src/intx.dart (right): https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:83: int compareTo(Comparable other); Nothing interesting to say here? https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:85: bool operator ==(other); Can we say anything here? Do we really want the superclass to show through here? https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:114: /** Returns `true` if and only if this integer less than zero. */ less -> is less https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:142: * Returns a [List] of [int], one per byte, starting with the least There's no need to link to the types, since you get that from the signature. Maybe: Returns a list of integers, one int per byte, ... OR Returns a List of int, ... https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:148: * Returns the [int] representation of this integer. [int] -> int https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:156: * Returns an [Int32] representation of this integer. [Int32] -> Int32 https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:163: /** Returns an [Int64] representation of this integer. */ [Int64] -> Int64 https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:166: /** Returns a decimal [String] representation of this integer. */ I had a hard time reading this at first. How about: Returns a string representing the decimal value of this integer; example: '13'. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:169: /** Returns a hexadecimal [String] representation of this integer. */ Maybe: Returns a string representing the hexadecimal value of this integer; example: '0xd'. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:173: * Returns a [String] representation of this integer in the given radix. Maybe: Returns a string representing the value of this integer in the given radix.
https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart File pkg/fixnum/lib/fixnum.dart (right): https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart... pkg/fixnum/lib/fixnum.dart:6: * This library provides fixed-size integer support. On 2013/08/14 17:06:51, justinfagnani wrote: > mention that it contains a 32 bit and 64 signed int? Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart... pkg/fixnum/lib/fixnum.dart:6: * This library provides fixed-size integer support. On 2013/08/14 17:15:07, Kathy Walrath wrote: > Our convention is for the first line to *not* be a complete sentence. How about > just: > > Fixed-size integer support. Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart... pkg/fixnum/lib/fixnum.dart:11: * ## Installing ## On 2013/08/14 17:06:51, justinfagnani wrote: > Do most libraries include pub install directions in the library docs? I would > think pub instructions should be left at the package level. I modelled these off the intl package. I agree though, README.md (which I've now added) seems like a better place for this. Thoughts? https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart... pkg/fixnum/lib/fixnum.dart:11: * ## Installing ## On 2013/08/14 17:15:07, Kathy Walrath wrote: > Why is there a ## after this header? Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart... pkg/fixnum/lib/fixnum.dart:13: * Use [pub][] to install this package. Add the following to your `pubspec.yaml` On 2013/08/14 17:15:07, Kathy Walrath wrote: > I'm not used to this style of linking. Is there a reason why you used this style > here and the []() style later? > > I'm inclined to use the []() style. Maybe I should put this in the style > guidelines unless there's a good reason not to. Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/fixnum.dart... pkg/fixnum/lib/fixnum.dart:14: * file. On 2013/08/14 17:15:07, Kathy Walrath wrote: > file. -> file: Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.dart File pkg/fixnum/lib/src/intx.dart (right): https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:12: /** Addition operator. */ On 2013/08/14 17:06:51, justinfagnani wrote: > Q for Kathy I guess: Should we doc the operators if they do the unsurprising > thing? It seems like documenting an implementation of a method defined in > another class. In Java we don't do that unless there's specific behavior that > differs from the declaration. Java doesn't provide operator overloads, but where it provides methods (e.g. BigInteger) they're documented. In dart:core, int documents these. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:83: int compareTo(Comparable other); On 2013/08/14 17:15:07, Kathy Walrath wrote: > Nothing interesting to say here? compareTo() docs are picked up off Comparable. I could provide detail more specific to intx & friends if useful. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:85: bool operator ==(other); On 2013/08/14 17:15:07, Kathy Walrath wrote: > Can we say anything here? Do we really want the superclass to show through here? Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:114: /** Returns `true` if and only if this integer less than zero. */ On 2013/08/14 17:15:07, Kathy Walrath wrote: > less -> is less Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:142: * Returns a [List] of [int], one per byte, starting with the least On 2013/08/14 17:15:07, Kathy Walrath wrote: > There's no need to link to the types, since you get that from the signature. > Maybe: > > Returns a list of integers, one int per byte, ... > OR > Returns a List of int, ... Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:148: * Returns the [int] representation of this integer. On 2013/08/14 17:15:07, Kathy Walrath wrote: > [int] -> int Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:156: * Returns an [Int32] representation of this integer. On 2013/08/14 17:15:07, Kathy Walrath wrote: > [Int32] -> Int32 Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:163: /** Returns an [Int64] representation of this integer. */ On 2013/08/14 17:15:07, Kathy Walrath wrote: > [Int64] -> Int64 Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:166: /** Returns a decimal [String] representation of this integer. */ On 2013/08/14 17:15:07, Kathy Walrath wrote: > I had a hard time reading this at first. How about: > > Returns a string representing the decimal value of this integer; > example: '13'. Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:169: /** Returns a hexadecimal [String] representation of this integer. */ On 2013/08/14 17:15:07, Kathy Walrath wrote: > Maybe: > > Returns a string representing the hexadecimal value of this integer; example: > '0xd'. Done. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:173: * Returns a [String] representation of this integer in the given radix. On 2013/08/14 17:15:07, Kathy Walrath wrote: > Maybe: > > Returns a string representing the value of this integer in the given radix. Done.
a few more nits... https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.dart File pkg/fixnum/lib/src/intx.dart (right): https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:12: /** Addition operator. */ I'm against documenting the obvious. (The exception is examples; those can be obvious, since they confirm that using this API really is as simple as it seems.) On 2013/08/14 17:06:51, justinfagnani wrote: > Q for Kathy I guess: Should we doc the operators if they do the unsurprising > thing? It seems like documenting an implementation of a method defined in > another class. In Java we don't do that unless there's specific behavior that > differs from the declaration. https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:83: int compareTo(Comparable other); Only if it's useful. On 2013/08/14 17:45:31, Chris Bracken wrote: > On 2013/08/14 17:15:07, Kathy Walrath wrote: > > Nothing interesting to say here? > > compareTo() docs are picked up off Comparable. I could provide detail more > specific to intx & friends if useful. https://codereview.chromium.org/23173002/diff/16001/pkg/fixnum/README.md File pkg/fixnum/README.md (right): https://codereview.chromium.org/23173002/diff/16001/pkg/fixnum/README.md#newc... pkg/fixnum/README.md:7: These integer implementations in this library are designed to work identically These -> The https://codereview.chromium.org/23173002/diff/16001/pkg/fixnum/lib/src/intx.dart File pkg/fixnum/lib/src/intx.dart (right): https://codereview.chromium.org/23173002/diff/16001/pkg/fixnum/lib/src/intx.d... pkg/fixnum/lib/src/intx.dart:170: * Returns a string representating the value of this integer in decimal representating!! -> representing (though I like representating) https://codereview.chromium.org/23173002/diff/16001/pkg/fixnum/lib/src/intx.d... pkg/fixnum/lib/src/intx.dart:171: * notation; example: `13`. shouldn't this have quotation marks, since it's a string literal? (we usually use single quotes, fwiw) https://codereview.chromium.org/23173002/diff/16001/pkg/fixnum/lib/src/intx.d... pkg/fixnum/lib/src/intx.dart:177: * notation; example: `0xd`. quotation marks?
https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.dart File pkg/fixnum/lib/src/intx.dart (right): https://codereview.chromium.org/23173002/diff/4001/pkg/fixnum/lib/src/intx.da... pkg/fixnum/lib/src/intx.dart:12: /** Addition operator. */ On 2013/08/14 17:55:28, Kathy Walrath wrote: > I'm against documenting the obvious. (The exception is examples; those can be > obvious, since they confirm that using this API really is as simple as it > seems.) > > On 2013/08/14 17:06:51, justinfagnani wrote: > > Q for Kathy I guess: Should we doc the operators if they do the unsurprising > > thing? It seems like documenting an implementation of a method defined in > > another class. In Java we don't do that unless there's specific behavior that > > differs from the declaration. > I think the real solution is for me to extend these docs slightly to cover over/underflow behaviour and type promotion. Will do that in a follow-up CL. https://codereview.chromium.org/23173002/diff/16001/pkg/fixnum/README.md File pkg/fixnum/README.md (right): https://codereview.chromium.org/23173002/diff/16001/pkg/fixnum/README.md#newc... pkg/fixnum/README.md:7: These integer implementations in this library are designed to work identically On 2013/08/14 17:55:28, Kathy Walrath wrote: > These -> The Done. https://codereview.chromium.org/23173002/diff/16001/pkg/fixnum/lib/src/intx.dart File pkg/fixnum/lib/src/intx.dart (right): https://codereview.chromium.org/23173002/diff/16001/pkg/fixnum/lib/src/intx.d... pkg/fixnum/lib/src/intx.dart:170: * Returns a string representating the value of this integer in decimal On 2013/08/14 17:55:28, Kathy Walrath wrote: > representating!! > -> > representing > > (though I like representating) Whoops - missed a couple backspaces. Done. https://codereview.chromium.org/23173002/diff/16001/pkg/fixnum/lib/src/intx.d... pkg/fixnum/lib/src/intx.dart:171: * notation; example: `13`. On 2013/08/14 17:55:28, Kathy Walrath wrote: > shouldn't this have quotation marks, since it's a string literal? > > (we usually use single quotes, fwiw) Done. https://codereview.chromium.org/23173002/diff/16001/pkg/fixnum/lib/src/intx.d... pkg/fixnum/lib/src/intx.dart:177: * notation; example: `0xd`. On 2013/08/14 17:55:28, Kathy Walrath wrote: > quotation marks? Done.
lgtm ...although I think we probably need to add a link to the pub page for the package. https://codereview.chromium.org/23173002/diff/23001/pkg/fixnum/lib/fixnum.dart File pkg/fixnum/lib/fixnum.dart (right): https://codereview.chromium.org/23173002/diff/23001/pkg/fixnum/lib/fixnum.dar... pkg/fixnum/lib/fixnum.dart:10: */ Shouldn't we have a link to the package homepage on pub.dartlang.org? Maybe: For information on getting this library, see the [fixnum package on pub.dartlang.org] (http://pub.dartlang.org/packages/fixnum).
https://codereview.chromium.org/23173002/diff/23001/pkg/fixnum/lib/fixnum.dart File pkg/fixnum/lib/fixnum.dart (right): https://codereview.chromium.org/23173002/diff/23001/pkg/fixnum/lib/fixnum.dar... pkg/fixnum/lib/fixnum.dart:10: */ On 2013/08/14 18:18:34, Kathy Walrath wrote: > Shouldn't we have a link to the package homepage on pub.dartlang.org? Maybe: > > For information on getting this library, see the > [fixnum package on pub.dartlang.org] > (http://pub.dartlang.org/packages/fixnum). Done.
Message was sent while issue was closed.
Committed patchset #4 manually as r26134 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
