[Do Not Merge] Update DocC documentation in the `PackageDescription` library. #4247

zkiraly · 2022-03-22T20:14:35Z

Update
This PR has been split into two. Please continue reviewing in those replacement PRs
Part 1 #4276
Part 2 #4277

Update DocC documentation in the PackageDescription library.

Motivation:

The DocC documentation in PackageDescription was not complete, and was out of sync with Apple's documentation at developer.apple.com.

Modifications:

Many DocC documentation comments were added or changed inside Sources/PackageDescription.

Result:

There was no code change. Only source code comments were changed in Sources/PackageDescription, and DocC extension files were added in the documentation catalog in that directory.

abertelrud · 2022-03-24T23:48:06Z

Sources/PackageDescription/LanguageStandardSettings.swift

    @available(_PackageDescription, introduced: 5.4)
    case gnu17

-    /// ISO C 2017 with GNU extensions.
+    /// The identifier for the GNU18 language standard.


Is the change in wording based on the identifier name? I think the thought was that the identifier name should be brief but the help text should explain the nuance about it being extensions on top of a standard. Same comment applies to the others in this enum.

I see what you mean. This is difficult, because GNU says that both gnu17 and gnu18 are "GNU dialect of ISO C17."

Anyway, I was following the pattern of the currently published documentation, which was reviewed and approved.

This is interesting, because #3079 changed the documentation from what's shown on Apple's documentation page. So, the documentation page is at least outdated by about 16 months.

abertelrud · 2022-03-24T23:48:51Z

Sources/PackageDescription/LanguageStandardSettings.swift

-///
-/// [1]: <https://clang.llvm.org/cxx_status.html>
+/// The supported C++ language standard to use for compiling C++ sources in the
+/// package.


These settings can be confusing so it might be beneficial to keep the middle paragraph and reference to the Clang documentation.

Yes, I agree. I'll put that back.

Sources/PackageDescription/Version.swift

WowbaggersLiquidLunch · 2022-03-25T07:26:51Z

Sources/PackageDescription/Version.swift

+
+    /// Returns a Boolean value indicating whether two values are equal.
+    ///
+    /// Equality is the inverse of inequality. For any values `a` and `b`, `a ==
+    /// b` implies that `a != b` is `false`.
+    ///
+    /// This is the default implementation of the equal-to operator (`==`) for
+    /// any type that conforms to `Equatable`.
+    ///
+    /// - Parameters:
+    ///   - lhs: A value to compare.
+    ///   - rhs: Another value to compare.
    @inlinable
    public static func == (lhs: Version, rhs: Version) -> Bool {
        !(lhs < rhs) && !(lhs > rhs)
    }
-
+
+    /// Returns a Boolean value indicating whether the value of the first
+    /// argument is less than that of the second argument.
+    ///
+    /// This function is the only requirement of the `Comparable` protocol. The
+    /// remainder of the relational operator functions are implemented by the
+    /// standard library for any type that conforms to `Comparable`.
+    ///
+    /// - Parameters:
+    ///   - lhs: A value to compare.
+    ///   - rhs: Another value to compare.


I think these are unnecessary, since the documentation should automatically inherit from Equatable's and Comparable's. However, it might be beneficial to replace the generic explanations of == and < with ones specific to semantic versions, with notes on things that users should watch for (e.g. build metadata identifiers, numeric vs. alphanumeric identifier precedence, pre-release precedence, etc).

Good point, definitely for the < function, I agree, because it's completely adapted for the purpose. I'll describe that in more detail. The others are more generic. I'll think about the Equatable conformance documentation, but for now, I am inclined to leave as-is.

Some of the description in =='s documentation doesn't make sense. For example, this line: "This is the default implementation of the equal-to operator (==) for any type that conforms to Equatable.", since Version is not using the default implementation of ==.

In general, I suggest that it's better to write the documentation for each function (not just these 2, but all functions in this PR) from scratch, instead of using the protocol's default implementation's, for the following reason:

If the conforming type is not using the default implementation, then the documentation for the default implementation likely is incorrect for the custom implementation.

If the conforming type is using the default implementation, then there is really no need to reuse the same documentation, because the same documentation should be automatically inherited.

WowbaggersLiquidLunch · 2022-03-25T07:30:24Z

Sources/PackageDescription/Version+StringLiteralConvertible.swift

-    /// - Parameter version: A string literal to use for creating a new version struct.
+    ///
+    /// - Parameters:
+    ///   - version: A string literal to use for creating a new version struct.


I think the current style where single paramenter uses - Parameter parameter: description is better, because its compactness reduces visual clatter around the code. However, it's only a matter of preference.

WowbaggersLiquidLunch · 2022-03-25T07:34:09Z

Sources/PackageDescription/Target.swift

@@ -14,10 +14,12 @@ import Foundation

 /// A target, the basic building block of a Swift package.
 ///
-/// Each target contains a set of source files that are compiled into a module or test suite.
-/// You can vend targets to other packages by defining products that include the targets.
+/// Each target contains a set of source files that Swift Package Manager compiles into a module


As far as I understand it, SwiftPM does not compile any code. Instead, it tells the driver and compiler what to compile and how to compile them.

My view is that that distinction is probably unnecessary to make for users of SwiftPM (as opposed to contributors to it such as ourselves).

That's fair, and perhaps SwiftPM can be considered as the compiler frontend's frontend.

Anyway, I think the current wording is better than the proposed change, since the passive voice hides the distinction, so it's both correct and friendly to the user.

Yes. This documentation is aimed at developers who use SwiftPM, and not at developers working on SwiftPM.

WowbaggersLiquidLunch · 2022-03-25T07:36:56Z

Sources/PackageDescription/SupportedPlatforms.swift

-/// A platform that usually corresponds to an operating system such as
-/// iOS, macOS, or Linux.
+/// A platform that corresponds to an operating system such as iOS,
+/// macOS, or Linux.


A platform is not always an OS, non-OS examples listed in the type include .driverKit and .wasi.

How about this:

A platform supported by Swift Package Manager.

WowbaggersLiquidLunch · 2022-03-25T07:43:18Z

Sources/PackageDescription/SupportedPlatforms.swift

-    /// Configures the minimum deployment target version for the macOS platform.
+    /// Configure the minimum deployment target version for the macOS platform.


The existing convention is using third-person present tense for a function's description. E.g. https://developer.apple.com/documentation/swift/array/1641390-remove

WowbaggersLiquidLunch · 2022-03-25T07:58:51Z

Sources/PackageDescription/PackageRequirement.swift

+    /// Returns a requirement for a version range, starting at the given minimum
+    /// version and going up to but not including the next major version. This is the recommended
+    /// version requirement.
+    ///
+    /// - Parameters:
+    ///   - version: The minimum version for the version range.


It's recommended to begin with a summary consisting only of a single sentence fragment. I would suggest something like this:

/// Returns a source control requirement bounded to the given version's major version number. /// /// Additional description and explanation here. /// - Parameter version: The given version to bound the source control requirement to. /// - Returns: A source control requirement bounded to `version`'s major version number.

WowbaggersLiquidLunch · 2022-03-25T08:01:16Z

Sources/PackageDescription/PackageRequirement.swift

-    /// visit [semver.org](https://semver.org).
+    /// visit the [Semantic Versioning 2.0.0](https://semver.org) website.


I think it's better to either leave it as it currently is, or update the URL to https://semver.org/spec/v2.0.0.html

I went with the second option, and I changed it in other places in PackageDescription also.

WowbaggersLiquidLunch · 2022-03-25T08:02:56Z

Sources/PackageDescription/PackageRequirement.swift

    @available(_PackageDescription, introduced: 999)
    public enum RegistryRequirement {
+        /// A version based requirement.


Since the other case is also a version-based requirement, perhaps it's more accurate to highlight the exact-ness of this case.

WowbaggersLiquidLunch · 2022-03-25T08:05:50Z

Sources/PackageDescription/PackageDescriptionSerialization.swift

+    /// If the value fails to encode anything, `encoder` will encode an empty
+    /// keyed container in its place.


I'm not sure if this description is correct.

I think this whole file is also an internal implementation details, i.e. manifests aren't expected to serialize anything. If this is documented, the impression is that manifests should do that. It's just that I don't think there is a way to hide the public codability conformance from the API.

WowbaggersLiquidLunch · 2022-03-25T08:13:07Z

Sources/PackageDescription/PackageDescription.swift

-/// The package manifest must begin with the string `// swift-tools-version:`,
-/// followed by a version number such as `// swift-tools-version:5.3`.
+/// The package manifest must begin with the string `// swift-tools-version:`
+/// followed by a version number specifier. The following code listing shows a
+/// few examples of valid declarations of the Swift tools version:


This rule has been relaxed since Swift 5.4. Manifest files for Swift ≥ 5.4 no long must begin with "// swift-tools-version:" followed immediately by the version string. They can contain any combination of whitespace before and after "//" and after ":".

Also the "swift-tools-version" part has always been case-insensitive, but the convention is all-lowercase.

That sounds too loosy-goosey to me. I'd rather not see people do

// swift-tool-version: 5.6

and the like. I understand that there may have been some benefit from the tooling point-of-view, but I don't know if it's beneficial to teach this.

We can mention the relaxation around whitespace without having to go to the extreme in recommending the format as shown in your example. One of the benefits is for the user to have more control with comments' alignment and format in their entire project. For example, I can imagine some users might prefer to have all their comments indented by some whitespace, starting with an uppercase letter, or aligned with doc comments by using 1 more spaces or a tab after "//". Providing them with this information allows them to treat the Swift version specification as a regular comment in their source code, and format it as they prefer.

WowbaggersLiquidLunch · 2022-03-25T08:27:24Z

Sources/PackageDescription/PackageDescription.swift

-/// Pass configuration options as parameters to your package's initializer
+/// Pass configuration options as parameters to your package’s initializer


Although it's correct to use U+2019 instead of U+0027 for punctuation, it can be more difficult to edit it using many IDE and keyboard combinations. (see page 270 of Unicode 13.0 spec which sort of outlines the dilemma)

This was due to my internal process, and I thank you for calling it out.

There are other locations where U+0027 is changed to U+2019. If the changes are not intended, then they should be changed back, too.

I changed all of them to the straight apostrophe under PackageDescription.

WowbaggersLiquidLunch · 2022-03-26T02:24:28Z

I wonder if it might be better to divide this PR into multiple smaller PRs, each focusing on just 1 or a few related files? There are a lot of lines changed in this PR. Smaller PRs can be easier (and perhaps faster) to review and prevent things from being overlooked.

…nts.

…9 to U+0027.

zkiraly · 2022-03-31T00:37:15Z

@WowbaggersLiquidLunch I split this PR into two: a PR dealing with Target, Product, and everything under those two (Part 2), and another PR (Part 1) dealing with everything else. I think then the two PRs might be just about equal in size.
Part 1 #4276
Part 2 #4277

zkiraly · 2022-04-23T15:41:52Z

The content was merged from the two split PRs. No further work on this PR. Closing.

Zsolt Kiraly and others added 3 commits March 22, 2022 12:20

Update DocC documentation.

e97dfc7

Merge branch 'main' into zkiraly/documentation-update

2e9fdb9

Removed blank line.

e4d56e3

abertelrud reviewed Mar 24, 2022

View reviewed changes

WowbaggersLiquidLunch reviewed Mar 25, 2022

View reviewed changes

Sources/PackageDescription/Version.swift Outdated Show resolved Hide resolved

WowbaggersLiquidLunch reviewed Mar 25, 2022

View reviewed changes

Sources/PackageDescription/Version.swift Outdated Show resolved Hide resolved

WowbaggersLiquidLunch reviewed Mar 25, 2022

View reviewed changes

Zsolt Kiraly added 2 commits March 28, 2022 18:45

Changes in response to WowbaggersLiquidLunch's and abertelrud's comme…

c6343ac

…nts.

Under PackageDescription, changed all instances (69 total) of U+201…

60f00c7

…9 to U+0027.

zkiraly changed the title ~~Update DocC documentation in the PackageDescription library.~~ [Do Not Merge] Update DocC documentation in the PackageDescription library. Mar 31, 2022

zkiraly added the DO NOT MERGE label Mar 31, 2022

WowbaggersLiquidLunch mentioned this pull request Mar 31, 2022

Part 1 of the PackageDescription DocC documentation update #4276

Merged

zkiraly closed this Apr 23, 2022

		/// Configures the minimum deployment target version for the macOS platform.
		/// Configure the minimum deployment target version for the macOS platform.

		/// visit [semver.org](https://semver.org).
		/// visit the [Semantic Versioning 2.0.0](https://semver.org) website.

		/// If the value fails to encode anything, `encoder` will encode an empty
		/// keyed container in its place.

		/// Pass configuration options as parameters to your package's initializer
		/// Pass configuration options as parameters to your package’s initializer

[Do Not Merge] Update DocC documentation in the PackageDescription library. #4247

[Do Not Merge] Update DocC documentation in the PackageDescription library. #4247

Uh oh!

Conversation

zkiraly commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation:

Modifications:

Result:

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!

Uh oh!

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!

abertelrud Mar 25, 2022 • 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!

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!

WowbaggersLiquidLunch Mar 25, 2022 • 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!

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!

WowbaggersLiquidLunch Mar 25, 2022 • 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!

WowbaggersLiquidLunch Mar 25, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

WowbaggersLiquidLunch commented Mar 26, 2022

Uh oh!

[Do Not Merge] Update DocC documentation in the `PackageDescription` library. #4247

[Do Not Merge] Update DocC documentation in the `PackageDescription` library. #4247

zkiraly commented Mar 22, 2022 •

edited

Loading

abertelrud Mar 25, 2022 •

edited

Loading

WowbaggersLiquidLunch Mar 25, 2022 •

edited

Loading

WowbaggersLiquidLunch Mar 25, 2022 •

edited

Loading

WowbaggersLiquidLunch Mar 25, 2022 •

edited

Loading