Clean up internal property nesting

[rauhul]

• Removes one layer of help properties by directly including the members
  of ArgumentHelp in ArgumentDefinition.Help. This also results in the
  discussion field which previously existed in both structures, now
  having a single source of truth. Adds helper method for setting each
  of these members using an instance of ArgumentHelp and additionally
  annotates each of the String? members with a new @NonEmpty property
  wrapper that ensures the wrapped string is either nil or nonempty so
  clients no do not need to check for the empty string case.

Replace this paragraph with a description of your changes and rationale. Provide links to an existing issue or external references/discussions, if appropriate.

Checklist

• I've added at least one test that validates that my change is working, if appropriate
• I've followed the code style of the rest of the project
• I've read the Contribution Guidelines
• I've updated the documentation if necessary

[rauhul]

@swift-ci Please test

[rauhul]

I'm sure about the @NonEmpty property wrapper. I would personally prefer using something like swift-nonempty by point free to embed this information in the typesystem. @natecook1000 thoughts?

[natecook1000]

Looks good, other than @NonEmpty, as you mentioned. Thanks, @rauhul!

[natecook1000]

Is there a semantic difference for these between "" and nil? If not, I'd rather make these non-optional and just use empty strings.

[rauhul]

I don't believe so, just that the property is derived from help: ArgumentHelp? so the optionality is inherited from there. I am happy to make these non-optional and make the points of use test for empty string. I think the only rough point is losing the ergonomics of optional chaining and defaulting. ex: valueName ?? otherValueName ?? "default"

[rauhul]

Example:

-    return help.valueName
-      ?? preferredNameForSynopsis?.valueString
+    return help.valueName.isEmpty
+      ? help.valueName
+      : preferredNameForSynopsis?.valueString
       ?? help.keys.first?.rawValue.convertedToSnakeCase(separator: "-")
       ?? "value"

[rauhul]

I think this is less desirable as it makes it easier to accidentally introduce bugs, like I just did in the above code. The condition should be *!*help.valueName.isEmpty

[natecook1000]

Yeah, the ergonomics are annoying, but I'd rather not keep them optional just for that purpose. That's kind of a funny usage point, since it's then lifting help.valueName into an optional to use with the following optional chaining… You could add a String extension like func mapEmpty(_ f: () -> String) -> String that would clear that up:

help.valueName.mapEmpty {
    preferredNameForSynopsis?.valueString
       ?? help.keys.first?.rawValue.convertedToSnakeCase(separator: "-")
       ?? "value"
}

[rauhul]

I went with your suggestion, but as an extension on Collection instead, since this replacement behavior seems generally useful.

---

It almost seems like there should be a NullValue protocol, such that this extension could operate on anything with a defined null and then the ?? operator could be extended to as well.

protocol NullValue {
  static var null: Self
}

extension NullValue where Self: Equatable {
  func mapNull(_ replacement: () -> Self) -> Self {
    self == null ? replacement() : self
  }
}

func ??<T: NullValue>(lhs: T, rhs: T) -> T {
  lhs.mapNull { rhs }
}

[natecook1000]

Yeah, it’s been proposed, I think!

[rauhul]

@swift-ci please test

[compnerd]

Hmm, this seems to have caused a regression on the Windows (bootstrap) build: https://dev.azure.com/compnerd/swift-build/_build/results?buildId=51233&view=logs&j=73ce2acc-4df7-56ec-1894-6d2e45aae46f&t=9f34e3dd-3373-5395-8997-8bd4a6b1fb09