Wrap HelpItems to table. (#629) #947

apogeeoak · 2020-06-26T15:44:27Z

Wrap HelpItems to table in HelpBuilder.

The column widths of the table are allocated to favor minimal rows. Wider columns are allocated more space if it is available.

Added tests to HelpBuilderTest to check that necessary sections properly wrap.

Example:
  Command description.

Usage:
  Example [options]

Options:
  --option-alias-for-a-command-th    Description that is long enough
  at-is-long-enough-to-wrap-to-a-    to wrap to a new line.
  new-line

Removed erroneous trailing whitespace in HelpBuilderTests.Help_describes_default_values_for_complex_root_command_scenario.approved.txt.

Additional auto-formatting in HelpBuilderTest.cs. Removed trailing whitespace and some argument alignment.

Fixes #629

dnfadmin · 2020-06-26T15:44:40Z

All CLA requirements met.

src/System.CommandLine/Help/HelpBuilder.cs

jonsequitur · 2020-06-26T16:15:21Z

src/System.CommandLine/Help/HelpBuilder.cs

+            return collection.Select(selector)
+                .Select(row => row
+                    .Select(element => RemoveFormatting(element))
+                    .ToList().AsReadOnly())


Is this needed? Does the the outer ToList suffice?

A ReadOnlyCollection is not necessary but I used one to follow the guidelines for collections.

I think a multidimensional array is better suited to represent a table. Originally I was going to implement the table view as a separate class with a multidimensional array backing but decided to implement it in place instead so I opted for a ReadOnlyCollection.

I believe that guidance predates the introduction of IReadOnlyCollection<T> and IReadOnlyList<T>. Generally, it's good to return a less specific type than a more specific type.

I'd avoid the multidimensional array as it's harder to reason about.

We have a table abstraction in System.CommandLine.Rendering. That project was started in part out of a desire to have better and more flexible help layouts. I wonder if there's an opportunity for reuse or convergence there.

@Keboo?

jonsequitur · 2020-06-26T16:16:01Z

src/System.CommandLine/Help/HelpBuilder.cs

+                return Array.AsReadOnly(Array.Empty<int>());
+
+            var columns = table.First().Count;
+            Debug.Assert(table.All(e => e.Count == columns), $"Every row in {nameof(table)} must have the same number of columns.");


Please remove the Debug.Assert.

I can remove the Debug.Assert but if ColumnWidths is called with an unequal number of columns an exception will be thrown at an indeterminate time. This should never happen in production so I thought an O(n) check for this was generally wasteful.

Would a compensation of some kind be preferable to an exception? Or can we make the incorrect state unrepresentable by the object model?

If the object model was a multidimensional array incorrect state would be unrepresentable as it would guarantee a consistent number of columns.

CreateTable will always return a table with a consistent number of columns but if ColumnWidths is called with arbitrary input then this cannot be guaranteed.

src/System.CommandLine/Help/HelpBuilder.cs

apogeeoak · 2020-06-26T19:54:14Z

I can squash the commits on approval.

jonsequitur · 2020-06-26T22:54:09Z

src/System.CommandLine/Help/HelpBuilder.cs

-            catch (System.IO.IOException)
-            {
+            if (console is SystemConsole systemConsole)
+                return systemConsole.GetConsoleWindowWidth();


Please add braces.

jonsequitur · 2020-06-29T17:28:58Z

Thanks, @apogeeoak! This is a really nice improvement.

apogeeoak · 2020-06-29T18:29:21Z

I would still like to make some changes to this and then squash the commits if possible.

I would like to add in the braces I forgot to add and change CreateTable to immediately evaluate and return an IReadOnlyList instead of an IEnumerable. I think the immediate evaluation is worth the added memory since the result is queried twice and may be an expensive operation. I was also considering changing the caching function from ToArray to ToList but I do not know which is better.

jonsequitur · 2020-06-29T19:08:03Z

Sorry for the over-eager merge. Further PRs to make optimizations are more than welcome.

apogeeoak · 2020-06-29T19:28:35Z

Should I create a new PR or reuse this one?

jonsequitur · 2020-06-29T19:34:43Z

Please create a new one.

Wrap HelpItems to table. (dotnet#629)

1e49943