network-comments: Added comments, fixed a few typos, and removed conditional parentheses for consistency #19108

agnosticdev · 2018-09-02T16:19:21Z

…itional parentheses for consistency

Added comments to public methods and variables.
Fixed a few typos.
Removed parentheses from conditionals for consistency.

No open Jira ticket currently for this PR.

…itional parentheses for consistency

xwu

Good catch on the typos!

Documentation for protocol methods is inherited, so please don't override them unless there's something additional to say specifically about that implementation, and if doing so make sure to copy the original documentation over and make any appropriate changes.

Also, please remove unrelated whitespace changes.

xwu · 2018-09-03T03:59:18Z

stdlib/public/SDK/Network/NWPath.swift

@@ -22,6 +22,7 @@ public struct NWInterface : Hashable, CustomDebugStringConvertible {
 		return self.name
 	}

+	/// Comparison function to determine the equality of two network interfaces by name and kernel index.


Protocol requirements inherit documentation also. Only override if there's something specific to add here; it is not necessary to point out that == is a comparison function: that is a semantic guarantee of Equatable (and therefore Hashable). See the Equatable protocol and elsewhere for standard phrasing for documentation. Here, it is sufficient simply to remove this comment altogether.

xwu · 2018-09-03T04:01:17Z

stdlib/public/SDK/Network/NWEndpoint.swift

 			let host = NWEndpoint.Host.name(String(cString: nw_endpoint_get_hostname(nw)), interface)
 			self = .hostPort(host: host, port: NWEndpoint.Port(nw_endpoint_get_port(nw)))
-		} else if (nw_endpoint_get_type(nw) == Network.nw_endpoint_type_address) {
+		} else if nw_endpoint_get_type(nw) == Network.nw_endpoint_type_address {
+


Please remove the added empty lines, here and above.

xwu · 2018-09-03T04:01:57Z

stdlib/public/SDK/Network/NWEndpoint.swift

@@ -630,31 +630,32 @@ public enum NWEndpoint: Hashable, CustomDebugStringConvertible {
 		if let nwinterface = nw_endpoint_copy_interface(nw) {
 			interface = NWInterface(nwinterface)
 		}
-		if (nw_endpoint_get_type(nw) == Network.nw_endpoint_type_host)
-		{
+		if nw_endpoint_get_type(nw) == Network.nw_endpoint_type_host{


If an opening brace is to be moved to the same line, a space should precede the brace.

xwu · 2018-09-03T04:02:14Z

stdlib/public/SDK/Network/NWEndpoint.swift

@@ -524,7 +524,7 @@ public enum NWEndpoint: Hashable, CustomDebugStringConvertible {
 				return interface
 			}
 		}
-
+		


Please revert this whitespace change.

xwu · 2018-09-03T04:06:07Z

stdlib/public/SDK/Network/NWEndpoint.swift

@@ -434,7 +434,7 @@ public struct IPv6Address: IPAddress, Hashable, CustomDebugStringConvertible {
 		hasher.combine(self.interface)
 	}

-	// CustomDebugStringConvertible
+	/// CustomDebugStringConvertible returning a debug description string of the IPv6 address and interface or just the address.


The original comment should either be preserved as it was or removed; again, documentation is inherited and one should not override unless there's something in particular to be stated about this implementation. Generally, the specific contents of the debug description are not written out in documentation as they may change.

xwu · 2018-09-03T04:08:22Z

stdlib/public/SDK/Network/NWEndpoint.swift

 		result = String(cString: storage)
 	}
 	storage.deallocate()
 	return result ?? "?"
 }

-/// An IP address
+/// An IP address protocol


Swift documentation style describes protocols by the types that can conform; the word "protocol" is not used. For example, BinaryInteger is documented as "An integer type with a binary representation." Here, IPAddress is appropriately documented as "An IP address." It is not a protocol of protocols of internet protocol addresses :)

xwu · 2018-09-03T04:09:11Z

stdlib/public/SDK/Network/NWConnection.swift

@@ -23,6 +23,7 @@ import _SwiftNetworkOverlayShims
 @available(macOS 10.14, iOS 12.0, watchOS 5.0, tvOS 12.0, *)
 public final class NWConnection : CustomDebugStringConvertible {

+	/// String based debugging representation of the NWConnection type.


Again here, do not override the inherited documentation for debugDescription unless there's something specific to be described.

xwu · 2018-09-03T04:10:54Z

stdlib/public/SDK/Network/NWConnection.swift

@@ -478,6 +479,7 @@ public final class NWConnection : CustomDebugStringConvertible {
 		}
 	}

+	/// Definition of callbacks used when sending data to the protocol stack. 


Please see existing documentation for the prevailing style. "Definition of" is not used for definitions.

xwu · 2018-09-03T04:13:30Z

stdlib/public/SDK/Network/NWPath.swift

@@ -74,7 +75,7 @@ public struct NWInterface : Hashable, CustomDebugStringConvertible {
 			}
 		}
 	}
-
+	// The interface type, such as other, wifi, cellular, wiredEthernet, and loopback.


Unlike the case for strings (below), it isn't necessary to give "such as" examples here as there are only those options; that is, after all, the purpose of using an enumeration.

I think you meant for this to be a doc comment?

xwu · 2018-09-03T04:13:40Z

stdlib/public/SDK/Network/NWPath.swift

@@ -74,7 +75,7 @@ public struct NWInterface : Hashable, CustomDebugStringConvertible {
 			}
 		}
 	}
-


Please revert this deleted line.

agnosticdev · 2018-09-03T13:26:09Z

@xwu Thank you for the feedback. Updating the PR now.

xwu · 2018-09-03T13:44:32Z

stdlib/public/SDK/Network/NWConnection.swift

@@ -478,6 +478,7 @@ public final class NWConnection : CustomDebugStringConvertible {
 		}
 	}

+	/// Callbacks used when sending data to the protocol stack. 


This should be singular. It’s a type that’s a lot like Optional, so the documentation should be as well. Something like: “A type that represents either a wrapped completion handler invoked when send content has been consumed by the protocol stack, or the lack of a completion handler because the content is idempotent.”

xwu · 2018-09-03T13:45:29Z

stdlib/public/SDK/Network/NWEndpoint.swift

-		if (nw_endpoint_get_type(nw) == Network.nw_endpoint_type_host)
-		{
+		if nw_endpoint_get_type(nw) == Network.nw_endpoint_type_host {
+


Please remove this empty line.

xwu · 2018-09-03T13:46:37Z

stdlib/public/SDK/Network/NWPath.swift

@@ -74,7 +74,8 @@ public struct NWInterface : Hashable, CustomDebugStringConvertible {
 			}
 		}
 	}
-
+
+	/// The interface types wifi, cellular, wiredEthernet, and loopback.


This should be singular; i.e., “The interface type.” Again, an enumeration doesn’t need to be enumerated in the documentation.

agnosticdev · 2018-09-03T14:02:46Z

Thank you @xwu. PR has been updated again.

xwu · 2018-09-03T14:33:26Z

stdlib/public/SDK/Network/NWPath.swift

@@ -75,6 +75,7 @@ public struct NWInterface : Hashable, CustomDebugStringConvertible {
 		}
 	}

+	/// The interface type wifi, cellular, wiredEthernet, and loopback.


Great. Last thing: to clarify my previous comment again, please remove "wifi, cellular, wiredEthernet, and loopback". That information is inherent in the fact that InterfaceType is an enumeration with only those cases. Moreover, it's documented in the comment for NWInterface itself.

agnosticdev · 2018-09-03T16:38:55Z

@xwu Thank you. PR updated.

moiseev

LGTM

moiseev · 2018-09-04T18:22:10Z

@swift-ci Please smoke test

network-comments: Added comments, fixed a few typos, and removed cond…

60f93e7

…itional parentheses for consistency

xwu reviewed Sep 3, 2018

View reviewed changes

network-comments: Updated PR based upon feedback received.

a76fb58

xwu reviewed Sep 3, 2018

View reviewed changes

network-comments: Updated PR based upon feedback received.

e943865

xwu reviewed Sep 3, 2018

View reviewed changes

network-comments: Updated PR based upon feedback received.

e30efb6

jrose-apple requested review from moiseev and lancep September 4, 2018 18:00

moiseev approved these changes Sep 4, 2018

View reviewed changes

lancep approved these changes Sep 4, 2018

View reviewed changes

moiseev merged commit ab04797 into swiftlang:master Sep 4, 2018

network-comments: Added comments, fixed a few typos, and removed conditional parentheses for consistency #19108

network-comments: Added comments, fixed a few typos, and removed conditional parentheses for consistency #19108

Uh oh!

Conversation

agnosticdev commented Sep 2, 2018

Uh oh!

xwu 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!

xwu Sep 3, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xwu Sep 3, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

agnosticdev commented Sep 3, 2018

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!

agnosticdev commented Sep 3, 2018

Uh oh!

xwu Sep 3, 2018 • 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!

agnosticdev commented Sep 3, 2018

Uh oh!

moiseev left a comment

Choose a reason for hiding this comment

Uh oh!

moiseev commented Sep 4, 2018

Uh oh!

Uh oh!

xwu Sep 3, 2018 •

edited

Loading

xwu Sep 3, 2018 •

edited

Loading

xwu Sep 3, 2018 •

edited

Loading