Fix swift package init --help printout #3407

miggs597 · 2021-04-15T18:38:18Z

Depends on this PR

Fixes swift package init --help to only show help for init
This is a major UI improvement as it allows the user to focus on the help that is solely relevant to init, and not get overwhelmed with other options.

rdar://76116106

tomerd · 2021-04-15T19:12:05Z

@swift-ci please smoke test

tomerd · 2021-04-15T20:07:03Z

@natecook1000 ptal

abertelrud

Thanks @miggs597, this is great!

So with the custom mirror, does this still depend on the ArgumentParser PR? Or are both needed, i.e. even with the ArgumentParser PR, will the custom mirror continue to be needed?

natecook1000 · 2021-04-21T22:51:11Z

Sources/Commands/SwiftPackageTool.swift

+
+        static let includeSuperCommandInHelp = false


We decided against requiring this bit for the ArgumentParser change.

Suggested change

static let includeSuperCommandInHelp = false

All of our subcommands need fixing. This is a demo of what fixing all the commands would look like Also includes some fillings for `Options()` that didn't have any help

miggs597 · 2021-04-22T16:13:14Z

To help us catch any possible regressions it would be nice to leverage AssertHelp.
PR

tomerd · 2021-04-22T17:19:19Z

Sources/Commands/SwiftPackageTool.swift

@@ -908,7 +914,7 @@ extension SwiftPackageTool {
        @OptionGroup()
        var swiftOptions: SwiftToolOptions

-        @Argument()
+        @Argument(help: "generate-bash-script | generate-zsh-script |\ngenerate-fish-script | list-dependencies | list-executables")


Can the argument parser library enumerate the enum options instead of having to explicitly spell them like this? cc @natecook1000

Maybe with ExpressibleByArgument, I know that it's responsible for filling in default: x

tomerd · 2021-04-22T17:20:37Z

Sources/Commands/SwiftPackageTool.swift

-            // FIXME: Error handling.
-            let cwd = localFileSystem.currentWorkingDirectory!
+            guard let cwd = localFileSystem.currentWorkingDirectory else {
+                throw StringError("Could not find the current working directory")


throw InternalError in such case, as it would assert in debug mode which is what we want in this failure case

tomerd · 2021-04-22T17:21:17Z

Sources/Commands/SwiftPackageTool.swift

@@ -190,22 +190,25 @@ extension SwiftPackageTool {
        }
    }

-    struct Init: SwiftCommand {
-        static let configuration = CommandConfiguration(
+    public struct Init: SwiftCommand {


why does this need to be public?

If we want to use AssertHelp we need access to the commands.

you can use @testable import to expose internal structures and functions for testing purposes. that is better than exposing more surface area in the public API.

tomerd · 2021-04-22T17:21:40Z

Tests/CommandsTests/PackageToolTests.swift

+        AssertHelp(for: Commands.SwiftPackageTool.Init.self, equals: """
+            OVERVIEW: Initialize a new package
+
+            USAGE: init [<options>] --enable-index-store


--enable-index-store ?

Yeah it's weird, but it does go away when they're all transitioned.

but it does go away when they're all transitioned. what does that mean?

another point is that I dont think its scalable to string match the the entire output of the --help for each command. did you intend to add such tests to all commands or just this one as an indicator for the rest?

It doesn't have to be for all the commands, I think it would be enough to have tests for the most popular commands: e.g. swift run, swift build, swift test

--enable-index-store goes away once all the commands have the option group hidden. In my initial testing that's when I noticed that it would go away.

miggs597 · 2021-04-26T00:38:08Z

@swift-ci please smoke test

Includes the new constructor that we need

tomerd · 2021-04-28T16:55:41Z

Package.swift

@@ -358,7 +358,7 @@ if ProcessInfo.processInfo.environment["SWIFTCI_USE_LOCAL_DEPS"] == nil {
        // The 'swift-argument-parser' version declared here must match that
        // used by 'swift-driver' and 'sourcekit-lsp'. Please coordinate
        // dependency version changes here with those projects.
-        .package(url: "https://github.com/apple/swift-argument-parser.git", .upToNextMinor(from: "0.3.1")),
+        .package(url: "https://github.com/apple/swift-argument-parser.git", .upToNextMinor(from: "0.4.3")),


please work @shahmishal to also update the toolchain CI definition

swiftlang/swift#37113

looks like this also needs to be coordinated with https://github.com/apple/swift-driver/blob/main/Package.swift#L134

cc @artemcm

tomerd · 2021-04-28T16:56:51Z

Sources/Commands/Options.swift

+            return .on
+        case .disableIndexStore:
+            return .off
+        }


this changes is a good one but we should do it in a separate PR

tomerd · 2021-04-28T16:56:58Z

Sources/Commands/Options.swift

+    }
+
+    @Flag(help: "Enable or disable indexing-while-building feature")
+    var indexStoreMode: StoreMode = .autoIndexStore


this changes is a good one but we should do it in a separate PR

This is what fixes USAGE: init [<options>] --enable-index-store preventing the index store from being printed out, should it still be another PR?

tomerd · 2021-04-28T16:57:16Z

Tests/CommandsTests/PackageToolTests.swift

@@ -12,7 +12,7 @@ import XCTest
 import Foundation

 import TSCBasic
-import Commands
+@testable import Commands


miggs597 · 2021-04-28T17:11:11Z

Updated Toolchain
@swift-ci please smoke test

miggs597 · 2021-04-28T18:15:25Z

swiftlang/swift/pull/37113
@swift-ci please smoke test

miggs597 · 2021-04-29T18:11:12Z

swiftlang/swift-driver#534
swiftlang/sourcekit-lsp#374
swiftlang/swift#37113
@swift-ci please smoke test

miggs597 · 2021-04-29T18:20:49Z

swiftlang/swift-driver#534
swiftlang/sourcekit-lsp#374
@swift-ci please smoke test

miggs597 · 2021-04-29T22:15:39Z

swiftlang/swift-driver#534
swiftlang/sourcekit-lsp#374
@swift-ci please smoke test

miggs597 · 2021-05-03T16:30:47Z

swiftlang/swift-driver#534
swiftlang/sourcekit-lsp#374
@swift-ci please smoke test

* Fix --help printout * What a fixed --help would look like All of our subcommands need fixing. This is a demo of what fixing all the commands would look like Also includes some fillings for `Options()` that didn't have any help * Testable import, and throw InternalError * Updated all remaining commands * Bump Swift-Argument-Parser Includes the new constructor that we need * Reverted import of Commands * Reverted indexStore, to be implemented in separate pr Co-authored-by: Miguel Perez <[email protected]> (cherry picked from commit a52d4d8)

* Fix --help printout * What a fixed --help would look like All of our subcommands need fixing. This is a demo of what fixing all the commands would look like Also includes some fillings for `Options()` that didn't have any help * Testable import, and throw InternalError * Updated all remaining commands * Bump Swift-Argument-Parser Includes the new constructor that we need * Reverted import of Commands * Reverted indexStore, to be implemented in separate pr Co-authored-by: Miguel Perez <[email protected]> (cherry picked from commit a52d4d8) Co-authored-by: tomer doron <[email protected]>

Fix --help printout

961425b

miggs597 requested review from abertelrud, friedbunny, neonichu and tomerd as code owners April 15, 2021 18:38

tomerd approved these changes Apr 15, 2021

View reviewed changes

abertelrud approved these changes Apr 15, 2021

View reviewed changes

tomerd mentioned this pull request Apr 16, 2021

Exclude supercommands from help apple/swift-argument-parser#300

Merged

4 tasks

abertelrud assigned miggs597 Apr 19, 2021

miggs597 mentioned this pull request Apr 19, 2021

Hide option group with new OptionGroup constructor apple/swift-argument-parser#301

Merged

4 tasks

natecook1000 reviewed Apr 21, 2021

View reviewed changes

What a fixed --help would look like

a15e484

All of our subcommands need fixing. This is a demo of what fixing all the commands would look like Also includes some fillings for `Options()` that didn't have any help

tomerd reviewed Apr 22, 2021

View reviewed changes

Testable import, and throw InternalError

876e69a

tomerd added the WIP Work in progress label Apr 22, 2021

Updated all remaining commands

b57a27f

neonichu approved these changes Apr 26, 2021

View reviewed changes

Bump Swift-Argument-Parser

6483303

Includes the new constructor that we need

tomerd reviewed Apr 28, 2021

View reviewed changes

shahmishal mentioned this pull request Apr 28, 2021

Update the swift-argument-parser for main and next branch to 0.4.3 swiftlang/swift#37113

Merged

neonichu mentioned this pull request Apr 29, 2021

Bump swift-argument-parser to 0.4.1 #3339

Closed

neonichu mentioned this pull request Apr 29, 2021

Bump swift-argument-parser to 0.4.1 swiftlang/sourcekit-lsp#374

Merged

Miguel Perez added 2 commits May 3, 2021 11:47

Reverted import of Commands

2f806ef

Reverted indexStore, to be implemented in separate pr

72e7009

neonichu merged commit a52d4d8 into swiftlang:main May 3, 2021

miggs597 mentioned this pull request May 5, 2021

[5.5] Fix swift package init --help printout #3473

Merged

Fix swift package init --help printout #3407

Fix swift package init --help printout #3407

Uh oh!

Conversation

miggs597 commented Apr 15, 2021

Uh oh!

tomerd commented Apr 15, 2021

Uh oh!

tomerd commented Apr 15, 2021

Uh oh!

abertelrud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miggs597 commented Apr 22, 2021

Uh oh!

tomerd Apr 22, 2021 • 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!

tomerd Apr 22, 2021 • 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!

tomerd Apr 22, 2021 • 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!

miggs597 commented Apr 26, 2021

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!

miggs597 commented Apr 28, 2021

Uh oh!

miggs597 commented Apr 28, 2021

Uh oh!

miggs597 commented Apr 29, 2021

Uh oh!

miggs597 commented Apr 29, 2021

Uh oh!

miggs597 commented Apr 29, 2021

Uh oh!

miggs597 commented May 3, 2021

Uh oh!

Uh oh!

tomerd Apr 22, 2021 •

edited

Loading

tomerd Apr 22, 2021 •

edited

Loading

tomerd Apr 22, 2021 •

edited

Loading