[stdlib] Various revisions and fixes for documentation #6311

natecook1000 · 2016-12-15T18:35:47Z

Fix wording for RandomAccessCollection
Add note about array growth to reserveCapacity(_:)
Reformat lazy flatMap discussions
Improve Collection symbol consistency

- Fix wording for RandomAccessCollection - Add note about array growth to reserveCapacity(_:) - Reformat lazy flatMap discussions - Improve Collection symbol consistency

natecook1000 · 2016-12-15T18:36:00Z

@swift-ci Please smoke test

stephentyrone

Looks good, except for two small style questions inline. I have only reviewed Arrays.swift.gyb, as the other stuff is outside my sphere of expertise.

stephentyrone · 2016-12-15T19:26:52Z

stdlib/public/core/Arrays.swift.gyb

+  ///
+  /// If you implement an custom data structure backed by an array that grows
+  /// dynamically, naively calling the `reserveCapacity(_:)` method can lead
+  /// to lower than expected performance. Arrays need to follow a geometric


Style question: "lower" or "worse"?

"worse" is certainly clearer

stephentyrone · 2016-12-15T19:29:07Z

stdlib/public/core/Arrays.swift.gyb

+  /// array on each call.
+  ///
+  ///       var values: [Int] = [0, 1, 2, 3]
+  ///


Do we have a standard method of marking code snippets that are showing "what not to do"? It would be nice to be explicit for someone skimming the text with a // don't do this, etc.

We don't really—there are cases where we mark that something won't compile, but haven't had cases quite like this. I added a comment.

stephentyrone · 2016-12-15T19:59:01Z

Thanks Nate. I'm happy with the Array section.

@airspeedswift please go ahead and approve once you review, if you like.

airspeedswift · 2016-12-15T20:59:52Z

stdlib/public/core/Arrays.swift.gyb

+  /// Preserving an Array's Geometric Growth Strategy
+  /// ===============================================
+  ///
+  /// If you implement an custom data structure backed by an array that grows


Gankra

Meta: is there any coherent rule for when docs should be duplicated from the "declaration" of a method and its various implementations in extensions?

Gankra · 2016-12-15T20:20:10Z

stdlib/public/core/Arrays.swift.gyb

+  /// Preserving an Array's Geometric Growth Strategy
+  /// ===============================================
+  ///
+  /// If you implement an custom data structure backed by an array that grows


Gankra · 2016-12-15T20:41:11Z

stdlib/public/core/FlatMap.swift

  ///
-  /// Use this method to receive only nonoptional values when your
+  /// Use this method to receive a sequence of nonoptional values when your
  /// transformation produces an optional value.


This sentence reads very weird, and maybe implies something stronger than we actually guarantee. e.g. if your transform yields T?? we don't produce a Sequence<T>, right?

Gankra · 2016-12-15T20:44:07Z

stdlib/public/core/FlatMap.swift

-  /// - Parameter transform: A closure that accepts an element of this
-  /// sequence as its argument and returns an optional value.
+  /// - Parameter transform: A closure that accepts an element of this sequence
+  ///   as its argument and returns an optional value.


Curiosity: what's our policy on doc'ing arguments? For instance, this one is completely vacuous, describing the type of the argument in english. Without knowing our policy, I would be generally inclined to either omit the description, or provide an actually meaningful one.

Gankra · 2016-12-15T20:45:43Z

stdlib/public/core/FlatMap.swift

-  /// Returns the concatenated results of mapping `transform` over
-  /// `self`.  Equivalent to 
+  /// Returns the concatenated results of mapping the given transformation over
+  /// this collection.


You're removing "concatenated" in some of these, and not others, any reasoning?

I'm only using "concatenated" when the result concatenates sequences/collections, not the optional unwrapping flatMap

Gankra · 2016-12-15T20:47:54Z

stdlib/public/core/FlatMap.swift

  ///
-  ///     self.map(transform).joined()
+  /// Use this method to receive a single-level collection when your


Knowing what flatmap is, I get what's meant by "single-level", but if I didn't, it would be pretty meaningless?

This also has the same ambiguity T?? had, but with [[[T]]].

Gankra · 2016-12-15T21:04:03Z

stdlib/public/core/RandomAccessCollection.swift

-  /// `limit` prevents offsetting beyond those bounds.
+  /// The value passed as `n` must not offset `i` beyond `endIndex` or before
+  /// `startIndex` for this collection, unless the index passed as `limit`
+  /// prevents offsetting beyond those bounds.


I honestly preferred the old wording. Is this an actual rule we have?

Gankra · 2016-12-15T21:06:17Z

stdlib/public/core/RandomAccessCollection.swift

@@ -269,9 +254,6 @@ where Index : Strideable,
  ///   If `n` is negative, this is the same value as the result of `-n` calls
  ///   to `index(before:)`.
  ///
-  /// - Precondition:
-  ///   - If `n > 0`, `n <= self.distance(from: i, to: self.endIndex)`
-  ///   - If `n < 0`, `n >= self.distance(from: i, to: self.startIndex)`


I'll admit that the above line is 100% more clear than this goop, but shouldn't the line above be listed as a "precondition"?

natecook1000 · 2016-12-19T19:10:05Z

@gankro Thanks for the flatMap feedback! I'm working on more comprehensive changes there since it shows up in a variety of places other than just the lazy collections. I'd like to merge this now essentially as a bug fix for the dangling "Equivalent to" in the discussion—I'll let you know when I have revised documentation for the rest.

natecook1000 · 2016-12-19T19:10:12Z

@swift-ci Please smoke test

airspeedswift · 2016-12-15T21:04:35Z

stdlib/public/core/Arrays.swift.gyb

+  /// If you implement an custom data structure backed by an array that grows
+  /// dynamically, naively calling the `reserveCapacity(_:)` method can lead
+  /// to worse than expected performance. Arrays need to follow a geometric
+  /// allocation pattern for appending elements to have amortized


"to have" reads slightly oddly in the sentence for me, maybe "to achieve" or "to get"

"to achieve" seems correct to me.

- Removed "see other" note from type discussion - Modified index(_:offsetBy...) to match other collections - Word choice in reserveCapacity(_:)

xwu · 2016-12-21T19:46:34Z

stdlib/public/core/RandomAccessCollection.swift

-  ///         c[i] /= 5
-  ///         i = c.index(after: i)
-  ///     }
-  ///     // c == MyFancyCollection([2, 4, 6, 8, 10])


Is there a reason for the elimination of this discussion?

Unlike other Indices types, a CountableRange<_> doesn't hold a reference to the collection, so that part of the discussion isn't accurate. It still shows up on the protocol requirement and the unconstrained indices implementations.

natecook1000 · 2016-12-22T05:04:35Z

@swift-ci Please smoke test and merge

[stdlib] Various revisions and fixes for documentation

3bc4909

- Fix wording for RandomAccessCollection - Add note about array growth to reserveCapacity(_:) - Reformat lazy flatMap discussions - Improve Collection symbol consistency

natecook1000 requested a review from stephentyrone December 15, 2016 19:25

stephentyrone reviewed Dec 15, 2016

View reviewed changes

airspeedswift self-requested a review December 15, 2016 19:48

[stdlib] Address feedback from @stephentyrone

f2bc719

airspeedswift reviewed Dec 15, 2016

View reviewed changes

Gankra reviewed Dec 15, 2016

View reviewed changes

[stdlib] Improvements to Collection doc comments

82811c5

airspeedswift reviewed Dec 19, 2016

View reviewed changes

[stdlib] Consistency fixes in Array documentation

4169635

- Removed "see other" note from type discussion - Modified index(_:offsetBy...) to match other collections - Word choice in reserveCapacity(_:)

xwu reviewed Dec 21, 2016

View reviewed changes

airspeedswift approved these changes Dec 22, 2016

View reviewed changes

swift-ci merged commit 6805ecc into swiftlang:master Dec 22, 2016

natecook1000 deleted the nc-fixes-06 branch December 22, 2016 21:19

[stdlib] Various revisions and fixes for documentation #6311

[stdlib] Various revisions and fixes for documentation #6311

Uh oh!

Conversation

natecook1000 commented Dec 15, 2016

Uh oh!

natecook1000 commented Dec 15, 2016

Uh oh!

stephentyrone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentyrone commented Dec 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gankra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natecook1000 commented Dec 19, 2016

Uh oh!

natecook1000 commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natecook1000 Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natecook1000 commented Dec 22, 2016

Uh oh!

Uh oh!

natecook1000 Dec 21, 2016 •

edited

Loading