review comments

Author: rnro

Package.swift

@@ -118,7 +118,7 @@ let targets: [Target] = [
 
   // Code generator SwiftPM command
   .plugin(
-    name: "GRPCGeneratorCommand",
+    name: "GRPCProtobufGeneratorCommand",
     capability: .command(
       intent: .custom(
         verb: "generate-grpc-code-from-protos",
@@ -132,7 +132,7 @@ let targets: [Target] = [
       ]
     ),
     dependencies: [
-      "protoc-gen-grpc-swift",
+      .target(name: "protoc-gen-grpc-swift"),
       .product(name: "protoc-gen-swift", package: "swift-protobuf"),
     ]
   ),

Plugins/GRPCGeneratorCommand/CommandConfig.swift renamed to Plugins/GRPCProtobufGeneratorCommand/CommandConfig.swift

@@ -20,6 +20,7 @@ import PackagePlugin
 struct CommandConfig {
   var common: GenerationConfig
 
+  var verbose: Bool
   var dryRun: Bool
 
   static let defaults = Self(
@@ -33,97 +34,103 @@ struct CommandConfig {
       importPaths: [],
       outputPath: ""
     ),
+    verbose: false,
     dryRun: false
   )
 }
 
 extension CommandConfig {
+  static func helpRequested(
+    argumentExtractor: inout ArgumentExtractor,
+  ) -> Bool {
+    let help = argumentExtractor.extractFlag(named: OptionsAndFlags.help.rawValue)
+    return help != 0
+  }
+
   static func parse(
-    arguments: [String],
+    argumentExtractor argExtractor: inout ArgumentExtractor,
     pluginWorkDirectory: URL
   ) throws -> (CommandConfig, [String]) {
     var config = CommandConfig.defaults
 
-    var argExtractor = ArgumentExtractor(arguments)
-
-    for flag in Flag.allCases {
+    for flag in OptionsAndFlags.allCases {
       switch flag {
       case .accessLevel:
         let accessLevel = argExtractor.extractOption(named: flag.rawValue)
-        if let value = accessLevel.first {
-          switch value.lowercased() {
-          case "internal":
-            config.common.accessLevel = .`internal`
-          case "public":
-            config.common.accessLevel = .`public`
-          case "package":
-            config.common.accessLevel = .`package`
-          default:
-            Diagnostics.error("Unknown accessLevel \(value)")
+        if let value = extractSingleValue(flag, values: accessLevel) {
+          if let accessLevel = GenerationConfig.AccessLevel(rawValue: value) {
+            config.common.accessLevel = accessLevel
           }
-        }
-      case .servers:
-        let servers = argExtractor.extractOption(named: flag.rawValue)
-        if let value = servers.first {
-          guard let servers = Bool(value) else {
-            throw CommandPluginError.invalidArgumentValue(value)
+          else {
+            Diagnostics.error("Unknown access level '--\(flag.rawValue)' \(value)")
           }
-          config.common.servers = servers
         }
-      case .clients:
-        let clients = argExtractor.extractOption(named: flag.rawValue)
-        if let value = clients.first {
-          guard let clients = Bool(value) else {
-            throw CommandPluginError.invalidArgumentValue(value)
-          }
-          config.common.clients = clients
+
+      case .servers, .noServers:
+        if flag == .noServers { continue }  // only process this once
+        let servers = argExtractor.extractFlag(named: OptionsAndFlags.servers.rawValue)
+        let noServers = argExtractor.extractFlag(named: OptionsAndFlags.noServers.rawValue)
+        if noServers > servers {
+          config.common.servers = false
         }
-      case .messages:
-        let messages = argExtractor.extractOption(named: flag.rawValue)
-        if let value = messages.first {
-          guard let messages = Bool(value) else {
-            throw CommandPluginError.invalidArgumentValue(value)
-          }
-          config.common.messages = messages
+
+      case .clients, .noClients:
+        if flag == .noClients { continue }  // only process this once
+        let clients = argExtractor.extractFlag(named: OptionsAndFlags.clients.rawValue)
+        let noClients = argExtractor.extractFlag(named: OptionsAndFlags.noClients.rawValue)
+        if noClients > clients {
+          config.common.clients = false
+        }
+
+      case .messages, .noMessages:
+        if flag == .noMessages { continue }  // only process this once
+        let messages = argExtractor.extractFlag(named: OptionsAndFlags.messages.rawValue)
+        let noMessages = argExtractor.extractFlag(named: OptionsAndFlags.noMessages.rawValue)
+        if noMessages > messages {
+          config.common.messages = false
         }
+
       case .fileNaming:
         let fileNaming = argExtractor.extractOption(named: flag.rawValue)
-        if let value = fileNaming.first {
-          switch value.lowercased() {
-          case "fullPath":
-            config.common.fileNaming = .fullPath
-          case "pathToUnderscores":
-            config.common.fileNaming = .pathToUnderscores
-          case "dropPath":
-            config.common.fileNaming = .dropPath
-          default:
-            Diagnostics.error("Unknown file naming strategy \(value)")
+        if let value = extractSingleValue(flag, values: fileNaming) {
+          if let fileNaming = GenerationConfig.FileNaming(rawValue: value) {
+            config.common.fileNaming = fileNaming
+          }
+          else {
+            Diagnostics.error("Unknown file naming strategy '--\(flag.rawValue)' \(value)")
           }
         }
+
       case .accessLevelOnImports:
         let accessLevelOnImports = argExtractor.extractOption(named: flag.rawValue)
-        if let value = accessLevelOnImports.first {
+        if let value = extractSingleValue(flag, values: accessLevelOnImports) {
           guard let accessLevelOnImports = Bool(value) else {
-            throw CommandPluginError.invalidArgumentValue(value)
+            throw CommandPluginError.invalidArgumentValue(name: flag.rawValue, value: value)
           }
           config.common.accessLevelOnImports = accessLevelOnImports
         }
+
       case .importPath:
         config.common.importPaths = argExtractor.extractOption(named: flag.rawValue)
+
       case .protocPath:
         let protocPath = argExtractor.extractOption(named: flag.rawValue)
-        config.common.protocPath = protocPath.first
+        config.common.protocPath = extractSingleValue(flag, values: protocPath)
+
       case .output:
         let output = argExtractor.extractOption(named: flag.rawValue)
-        config.common.outputPath = output.first ?? pluginWorkDirectory.absoluteStringNoScheme
+        config.common.outputPath = extractSingleValue(flag, values: output) ?? pluginWorkDirectory.absoluteStringNoScheme
+
+      case .verbose:
+        let verbose = argExtractor.extractFlag(named: flag.rawValue)
+        config.verbose = verbose != 0
+
       case .dryRun:
         let dryRun = argExtractor.extractFlag(named: flag.rawValue)
         config.dryRun = dryRun != 0
+
       case .help:
-        let help = argExtractor.extractFlag(named: flag.rawValue)
-        if help != 0 {
-          throw CommandPluginError.helpRequested
-        }
+        () // handled elsewhere
       }
     }
 
@@ -141,19 +148,30 @@ extension CommandConfig {
   }
 }
 
+func extractSingleValue(_ flag: OptionsAndFlags, values: [String]) -> String? {
+  if values.count > 1 {
+    Stderr.print("Warning: '--\(flag.rawValue)' was unexpectedly repeated, the first value will be used.")
+  }
+  return values.first
+}
+
 /// All valid input options/flags
-enum Flag: CaseIterable, RawRepresentable {
+enum OptionsAndFlags: CaseIterable, RawRepresentable {
   typealias RawValue = String
 
   case servers
+  case noServers
   case clients
+  case noClients
   case messages
+  case noMessages
   case fileNaming
   case accessLevel
   case accessLevelOnImports
   case importPath
   case protocPath
   case output
+  case verbose
   case dryRun
 
   case help
@@ -162,22 +180,30 @@ enum Flag: CaseIterable, RawRepresentable {
     switch rawValue {
     case "servers":
       self = .servers
+    case "no-servers":
+      self = .noServers
     case "clients":
       self = .clients
+    case "no-clients":
+      self = .noClients
     case "messages":
       self = .messages
+    case "no-messages":
+      self = .noMessages
     case "file-naming":
       self = .fileNaming
     case "access-level":
       self = .accessLevel
-    case "use-access-level-on-imports":
+    case "access-level-on-imports":
       self = .accessLevelOnImports
     case "import-path":
       self = .importPath
     case "protoc-path":
       self = .protocPath
     case "output":
       self = .output
+    case "verbose":
+      self = .verbose
     case "dry-run":
       self = .dryRun
     case "help":
@@ -192,10 +218,16 @@ enum Flag: CaseIterable, RawRepresentable {
     switch self {
     case .servers:
       "servers"
+    case .noServers:
+      "no-servers"
     case .clients:
       "clients"
+    case .noClients:
+      "no-clients"
     case .messages:
       "messages"
+    case .noMessages:
+      "no-messages"
     case .fileNaming:
       "file-naming"
     case .accessLevel:
@@ -208,6 +240,8 @@ enum Flag: CaseIterable, RawRepresentable {
       "protoc-path"
     case .output:
       "output"
+    case .verbose:
+      "verbose"
     case .dryRun:
       "dry-run"
     case .help:
@@ -216,15 +250,21 @@ enum Flag: CaseIterable, RawRepresentable {
   }
 }
 
-extension Flag {
+extension OptionsAndFlags {
   func usageDescription() -> String {
     switch self {
     case .servers:
-      return "Whether server code is generated. Defaults to true."
+      return "Indicate that server code is to be generated. Generated by default."
+    case .noServers:
+      return "Indicate that server code is not to be generated. Generated by default."
     case .clients:
-      return "Whether client code is generated. Defaults to true."
+      return "Indicate that client code is to be generated. Generated by default."
+    case .noClients:
+      return "Indicate that client code is not to be generated. Generated by default."
     case .messages:
-      return "Whether message code is generated. Defaults to true."
+      return "Indicate that message code is to be generated. Generated by default."
+    case .noMessages:
+      return "Indicate that message code is not to be generated. Generated by default."
     case .fileNaming:
       return
         "The naming scheme for output files [fullPath/pathToUnderscores/dropPath]. Defaults to fullPath."
@@ -236,27 +276,36 @@ extension Flag {
     case .importPath:
       return "The directory in which to search for imports."
     case .protocPath:
-      return "The path to the `protoc` binary."
+      return "The path to the protoc binary."
     case .dryRun:
       return "Print but do not execute the protoc commands."
     case .output:
       return "The path into which the generated source files are created."
+    case .verbose:
+      return "Emit verbose output."
     case .help:
       return "Print this help."
     }
   }
 
-  static func printHelp() {
-    print("Usage: swift package generate-grpc-code-from-protos [flags] [input files]")
-    print("")
-    print("Flags:")
-    print("")
+  static func printHelp(requested: Bool) {
+    let printMessage: (String) -> Void
+    if requested {
+      printMessage = { message in print(message) }
+    } else {
+      printMessage = Stderr.print
+    }
+
+    printMessage("Usage: swift package generate-grpc-code-from-protos [flags] [input files]")
+    printMessage("")
+    printMessage("Flags:")
+    printMessage("")
 
     let spacing = 3
     let maxLength =
-      (Flag.allCases.map(\.rawValue).max(by: { $0.count < $1.count })?.count ?? 0) + spacing
-    for flag in Flag.allCases {
-      print(
+      (OptionsAndFlags.allCases.map(\.rawValue).max(by: { $0.count < $1.count })?.count ?? 0) + spacing
+    for flag in OptionsAndFlags.allCases {
+      printMessage(
         "  --\(flag.rawValue.padding(toLength: maxLength, withPad: " ", startingAt: 0))\(flag.usageDescription())"
       )
     }

Plugins/GRPCGeneratorCommand/CommandPluginError.swift renamed to Plugins/GRPCProtobufGeneratorCommand/CommandPluginError.swift

@@ -15,22 +15,19 @@
  */
 
 enum CommandPluginError: Error {
-  case helpRequested
   case missingArgumentValue
-  case invalidArgumentValue(String)
+  case invalidArgumentValue(name: String, value: String)
   case missingInputFile
   case unknownOption(String)
 }
 
 extension CommandPluginError: CustomStringConvertible {
   var description: String {
     switch self {
-    case .helpRequested:
-      "User requested help."
     case .missingArgumentValue:
       "Provided option does not have a value."
-    case .invalidArgumentValue:
-      "Invalid option value."
+    case .invalidArgumentValue(let name, let value):
+      "Invalid value '\(value)', for '\(name)'."
     case .missingInputFile:
       "No input file(s) specified."
     case .unknownOption(let value):