Skip to content

Commit

Permalink
Fixed all the bugs in reflow that I found, removed all hacks
Browse files Browse the repository at this point in the history
  • Loading branch information
@scottbilas
scottbilas committed Nov 2, 2023
1 parent 6b21a75 commit 89e896e
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 105 deletions.
66 changes: 46 additions & 20 deletions src/Core/DocoptUtility.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ public class DocoptReflowOptions
[PublicAPI]
public static class DocoptUtility
{
// TODO: nothing about this (once the "Usage" hack is removed) should be specific to docopt.
// TODO: nothing about this should be specific to docopt.
// can use blank lines to mean sections, detecting alignment, and - most importantly - the
// "end of last double space" rule to mean the indent point for wrapping.

Expand All @@ -40,7 +40,7 @@ public static string Reflow(string text, int wrapWidth, DocoptReflowOptions opti
var result = new StringBuilder();

var needEol = false;
foreach (var section in SelectSections(text))
foreach (var section in SelectSections(text).ToArray()) // ToArray is to force _extraIndent to work (needs to modify the prev)
{
if (needEol)
{
Expand All @@ -56,12 +56,12 @@ public static string Reflow(string text, int wrapWidth, DocoptReflowOptions opti

// do the indent here so its whitespace doesn't get caught up in the calculations below
// (TODO: this breaks very narrow wrapping..)
result.Append(section.Span.WithLength(section.Indent).Span);
var span = section.Span.WithOffsetStart(section.Indent);
result.Append(section.Span.WithLength(section.TotalIndent).Span);
var span = section.Span.WithOffsetStart(section.TotalIndent);

// special: if we have a really wide column indent, let's fall back to non aligned
// (TODO: also broken with very narrow wrapping..)
var indent = section.Indent;
var indent = section.TotalIndent;
if (options.IndentFallback != 0 && wrapWidth - indent < options.IndentFallback)
{
indent = section.Span.TrimStartOffset()+1;
Expand Down Expand Up @@ -101,24 +101,51 @@ public static string Reflow(string text, int wrapWidth, DocoptReflowOptions opti
return result.ToString();
}

record struct Section(StringSpan Span, int Indent)
record Section(StringSpan Span, int Indent)
{
int _extraIndent;

public Section(StringSpan span) : this(span.TrimEnd(), 0)
{
var indentMatch = Span.Match(s_indentRx);
var indentMatch = Span.Match(s_indentRx0);
if (indentMatch.Success)
Indent = indentMatch.Index - Span.Start + indentMatch.Length;
else
Indent = Span.TrimStartOffset();
{
indentMatch = Span.Match(s_indentRx1);
if (indentMatch.Success)
Indent = indentMatch.Index - Span.Start + indentMatch.Length;
else
Indent = Span.TrimStartOffset();
}
}

bool HasPrefix => Span.TrimStartOffset() < Indent;
public int TotalIndent => Indent + _extraIndent;
bool HasPrefix => Span.TrimStartOffset() < Indent; // TODO: "extra indent" and "prefix" concepts do the same thing; join them

// note that this may modify the previous section if we detect duplicate leading words
public Section? MergeWith(Section other)
{
if (Span.IsEmpty || other.Span.IsEmpty || Indent != other.Indent || other.HasPrefix)
return null;

if (!HasPrefix)
{
// if the leading word is identical (common with program name in 'usage' lines), do not merge
var end = Span[Indent..].IndexOf(' ');
if (end >= 0)
{
var wordLen = end + 1; // include the space
var span0 = Span[Indent..].WithLength(wordLen);
var span1 = other.Span[Indent..].WithLength(wordLen);
if (span0.TextEquals(span1))
{
_extraIndent = other._extraIndent = wordLen;
return null;
}
}
}

// $$$ TODO: get rid of this silliness
var newText = Span.ToString() + ' ' + other.Span.TrimStart();
return this with { Span = new StringSpan(newText) };
Expand All @@ -127,7 +154,12 @@ public Section(StringSpan span) : this(span.TrimEnd(), 0)
public override string ToString() => $"{Span.ToDebugString()}; indent={Indent}, prefix={HasPrefix}";
}

static readonly Regex s_indentRx = new(@"^ *([-*]|\d+\.) |\b {2,}\b");
// these regexes find where we should indent to upon wrapping, in priority order.
//
// - align to the right side of a "docopt divider" (>= 2 spaces). higher pri to catch bulleted option lists.
static readonly Regex s_indentRx0 = new(@"\S {2,}");
// - align to the text part of a '*' or '-' style bullet point
static readonly Regex s_indentRx1 = new(@"^ *([-*]|\d+\.) ");

static IEnumerable<StringSpan> SelectLines(string text)
{
Expand All @@ -150,24 +182,18 @@ static IEnumerable<Section> SelectSections(string text)
{
Section? lastSection = null;

var inUsage = false; // TODO: remove this hack. there's a bug where the usage section tends to get combined into one line. fix that (see Reflow_Bug_UsageLinesGettingJoined), then remove this!
foreach (var span in SelectLines(text))
{
var newSection = new Section(span);

if (span == "Usage:")
inUsage = true;
else if (span.Length != 0 && span[0] != ' ')
inUsage = false;

if (lastSection != null)
{
var merged = lastSection.Value.MergeWith(newSection);
if (!inUsage && merged != null)
var merged = lastSection.MergeWith(newSection);
if (merged != null)
lastSection = merged;
else
{
yield return lastSection.Value;
yield return lastSection;
lastSection = newSection;
}
}
Expand All @@ -176,6 +202,6 @@ static IEnumerable<Section> SelectSections(string text)
}

if (lastSection != null)
yield return lastSection.Value;
yield return lastSection;
}
}
121 changes: 37 additions & 84 deletions src/Core/DocoptUtility.t.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,34 +64,6 @@ public void Reflow_WithSimpleContinuations()
"this is a\nline we are\nwrapping and\nanother that\nshould be\njoined to it");
}

[Test]
public void Reflow_WithExistingWhitespaceBreaks()
{
// baseline
Reflow(
"this is a line we r wrapping", 12).ShouldBe(
"this is a\nline we r\nwrapping");
Reflow(
"this is a1 line we r wrapping", 12).ShouldBe(
"this is a1\nline we r\nwrapping");
Reflow(
"this is a12 line we r wrapping", 12).ShouldBe(
"this is a12\nline we r\nwrapping");
Reflow(
"this is a123 line we r wrapping", 12).ShouldBe(
"this is a123\nline we r\nwrapping");

// collapse whitespace when wrapped
Reflow(
"* this is a line we r wrapping ", 14).ShouldBe(
"* this is a\n line we r\n wrapping");

// preserve whitespace when not wrapped
Reflow(
"- this is a line we r wrapping ", 20).ShouldBe(
"- this is a line\n we r wrapping");
}

[Test]
public void Reflow_WithSimpleIndentation()
{
Expand Down Expand Up @@ -160,6 +132,16 @@ public void Reflow_WithBulletPoints()
Reflow(
"here is\n * one point\n and this should join", 50).ShouldBe(
"here is\n * one point and this should join");

Reflow(
"here is\n - one point\n and this won't! join", 16).ShouldBe(
"here is\n - one point\n and this\n won't! join");
Reflow(
"here is\n - one point\n and this won't! join", 18).ShouldBe(
"here is\n - one point and\n this won't!\n join");
Reflow(
"here is\n - one point\n and this should join", 50).ShouldBe(
"here is\n - one point and this should join");
}

[Test]
Expand Down Expand Up @@ -189,10 +171,11 @@ public void Reflow_LongIndentedWord_ShouldNotBreakOnLeadingWhitespace()
" this_is_a_r\n eally_long_\n word");
}

[Test, Category("TODO")]
public void Reflow_Bug_FinalLineNotIndented()
[Test, Category("Regression")]
public void Reflow_FinalLineIndented()
{
// TODO: something about the '--version' in there is causing the bad wrapping
// before the fix, this would fail to indent the very last line

Reflow(
"Usage:\n"+
" okflog PATH long stuff on top of it\n"+
Expand All @@ -201,85 +184,55 @@ public void Reflow_Bug_FinalLineNotIndented()
"Usage:\n"+
" okflog PATH long stuff on top\n"+
" of it\n"+
// WHAT WE WANT
// " okflog --version long stuff on\n"+
// " top of it"
// WHAT WE ACTUALLY GET
" okflog --version long stuff\n"+
" on top of it"
);
" on top of it");
}

[Test, Category("TODO"), Ignore("need bugfix, hacked around for now")]
[Test]
public void Reflow_WithProgramUsage_DoesNotJoinLines()
{
// TODO: once this bug is resolved, remove the hack from DocoptUtility.SelectSections, and also update --no-hub to be like below formatting

// TODO: something about the '--version' in there is causing the bad wrapping
// UPDATE: this is probably caused by the bracket. changing "[options]" to "options" probably won't show the problem.
// i ran into this with a "[windows-only]" prefix on some options help text and removing the brackets fixed it. same issue happens with parens..
// so it's something easy with the `\b` in s_indentRx that's causing the problem.
// --no-hub [windows-only] Run `okunity do hidehub --kill-hub` before launching Unity, which will kill the Hub if running and also prevent the auto-launch of the Hub that Unity does (note that this change has global impact, check `help do` for more info on this)

Reflow(
"Usage:\n"+
" loggo [options] [DESTINATION]\n"+
" loggo --version\n",
" loggo --version",
50).ShouldBe(
// WHAT WE WANT
// "Usage:\n"+
// " loggo [options] [DESTINATION]\n"+
// " loggo --version",
// WHAT WE ACTUALLY GET (without the hack)
"Usage:\n"+
" loggo [options] [DESTINATION] loggo --version"
);

// these tests worked before i hacked out the "usage" wrapping.

// (previously i could double-space after the program name to work around the issue, but that started failing
// at some point, leading to the above false test.)
" loggo [options] [DESTINATION]\n"+
" loggo --version");
}

#if NO
Reflow(
"Usage:\n progname and some extra text\n progname and some other text", 23).ShouldBe(
// TODO: what i want to work
// "Usage:\n progname and some\n extra text\n progname and some\n other text");
// what currently happens
"Usage:\n progname and some\n extra text progname\n and some other text");
[Test]
public void Reflow_WithProgramUsage_WrapsLinesIndividually()
{
// this tests out the section "leading word" detection and extra indent adjusting

// this gets it right, but requires a workaround of double-space after program name (not the end of the world)
Reflow(
"Usage:\n progname and some extra text\n progname and some other text", 23).ShouldBe(
"Usage:\n progname and some\n extra text\n progname and some\n other text");
#endif

"Usage:\n"+
" progname and some extra text\n"+
" progname and some other text",
23).ShouldBe(
"Usage:\n"+
" progname and some\n"+
" extra text\n"+
" progname and some\n"+
" other text");
}

[Test, Category("TODO")]
public void Reflow_WithDoubleSpace_IndentsAtDoubleSpace()
[Test]
public void Reflow_WithDoubleSpace_PrefersIndentAtDoubleSpace()
{
Reflow(
"Options:\n"+
" --thingy This is a really long line that should be wrapping at the double space\n"+
" * first: This one should not wrap at '* ' but instead at 'This'\n"+
" * second: This one should also wrap at the 'This' and not before that\n",
50).ShouldBe(
// WHAT WE WANT
// "Options:\n"+
// " --thingy This is a really long line that should\n"+
// " be wrapping at the double space\n"+
// " * first: This one should not wrap at '* '\n"+
// " but instead at 'This'\n"+
// " * second: This one should also wrap at the\n"+
// " 'This' and not before that");
// WHAT WE ACTUALLY GET
"Options:\n"+
" --thingy This is a really long line that should\n"+
" be wrapping at the double space\n"+
" * first: This one should not wrap at '* '\n"+
" but instead at 'This'\n"+
" but instead at 'This'\n"+
" * second: This one should also wrap at the\n"+
" 'This' and not before that");
" 'This' and not before that");
}
}
2 changes: 1 addition & 1 deletion src/Unity.Cli/Commands_RunUnity.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ Any arguments given in EXTRA will be passed along to the newly launched Unity pr
--pid-exitcode Return the Unity process ID as the exit code (*)
--job-worker-count JWC Set a limit on both a) job worker thread count and b) shader compiler process count; JWC can be either X to choose an explicit number or X% to choose a percentage of machine vCPU's
--no-cache-server Tell Unity not to use the cache server
--no-hub windows-only: Run `okunity do hidehub --kill-hub` before launching Unity, which will kill the Hub if running and also prevent the auto-launch of the Hub that Unity does (note that this change has global impact, check `help do` for more info on this)
--no-hub [windows-only] Run `okunity do hidehub --kill-hub` before launching Unity, which will kill the Hub if running and also prevent the auto-launch of the Hub that Unity does (note that this change has global impact, check `help do` for more info on this)
--no-burst Completely disable Burst
--no-activate-existing Don't activate an existing Unity main window if found running on the project
Expand Down

0 comments on commit 89e896e

Please sign in to comment.