From 63898c1bb5e2761385864bf6fc89a859aa606e7e Mon Sep 17 00:00:00 2001 From: James Luck Date: Mon, 13 Feb 2023 15:45:34 +1100 Subject: [PATCH] Improving how we pretty print by trying to limit line length and avoid eagerly breaking lines (#13) Co-authored-by: James Luck --- src/PromQL.Parser/Printer.cs | 93 ++++++++++++++++------- src/PromQL.Parser/PromQL.Parser.csproj | 2 +- tests/PromQL.Parser.Tests/PrinterTests.cs | 70 +++++++++++++---- 3 files changed, 124 insertions(+), 41 deletions(-) diff --git a/src/PromQL.Parser/Printer.cs b/src/PromQL.Parser/Printer.cs index f3f6fe6..1987cdc 100644 --- a/src/PromQL.Parser/Printer.cs +++ b/src/PromQL.Parser/Printer.cs @@ -24,7 +24,7 @@ public class Printer : IVisitor public Printer(PrinterOptions? options = null) { _options = options ?? PrinterOptions.PrettyDefault; - _sb = new IndentedStringBuilder(_options.IndentChar, _options.IndentCount); + _sb = new IndentedStringBuilder(_options.IndentChar, _options.IndentCount, _options.BreakAfterCharCount); } public virtual void Visit(StringLiteral s) @@ -146,7 +146,7 @@ public virtual void Visit(ParenExpression paren) { Write("("); - using (_options.BreakOnParenExpr ? _sb.IncreaseIndent() : null) + using (_options.BreakOnParenExpr ? _sb.StartScope() : null) paren.Expr.Accept(this); Write(")"); @@ -209,12 +209,11 @@ public virtual void Visit(VectorMatching vm) public virtual void Visit(BinaryExpr expr) { expr.LeftHandSide.Accept(this); + Write(" "); if (_options.BreakOnBinaryOperators) - _sb.AppendLine(); - else - Write(" "); - + _sb.AddPotentialBreakPoint(); + Write($"{expr.Operator.ToPromQl()} "); var preLen = _sb.Length; @@ -259,7 +258,8 @@ public string ToPromQl(IPromQlNode node) throw new ArgumentNullException(); _sb.Clear(); - node.Accept(this); + using(_sb.StartScope()) + node.Accept(this); return _sb.ToString()!; } @@ -267,48 +267,86 @@ public string ToPromQl(IPromQlNode node) internal class IndentedStringBuilder { - private int _indentLevel = 0; private readonly string[] _indents; private const int MaxIndent = 20; private StringBuilder _sb = new StringBuilder(); - - public IndentedStringBuilder(char indentChar, int indentCount) + private Stack _scopes = new Stack(); + private readonly int _idealLineLength; + + public IndentedStringBuilder(char indentChar, int indentCount, int idealLineLength) { + _idealLineLength = idealLineLength; _indents = Enumerable.Range(0, MaxIndent) - .Select(n => new string(Enumerable.Repeat(indentChar, indentCount * n).ToArray())) + .Select(n => Environment.NewLine + new string(Enumerable.Repeat(indentChar, indentCount * n).ToArray())) .ToArray(); } - public void Append(string? s) => _sb.Append(s); - + public void Append(string? s) + { + CheckForScope(); + _sb.Append(s); + } + public int Length => _sb.Length; public void Clear() => _sb.Clear(); - public void AppendLine() + private void AppendLine(int indent) { - _sb.AppendLine(); - _sb.Append(_indents[_indentLevel]); + _sb.Append(_indents[indent]); } public override string ToString() => _sb.ToString(); - public IDisposable IncreaseIndent() + public IDisposable StartScope() { - _indentLevel++; - if (_indentLevel == MaxIndent) + if (_scopes.Count == MaxIndent) throw new InvalidOperationException($"Cannot increase indent beyond {MaxIndent}"); - AppendLine(); + _scopes.Push(new Scope(_sb.Length)); + return new DisposableIndent(this); } - public void DecreaseIndent() + public void AddPotentialBreakPoint() => _scopes.Peek().BreakPositions.Add(_sb.Length); + + public void EndScope() + { + var indentLevel = _scopes.Count - 1; + var scope = _scopes.Pop(); + + if (_sb.Length - scope.StartIndex <= _idealLineLength) + return; + + int insertedIndent = 0; + + if (scope.StartIndex > 0) + { + _sb.Insert(scope.StartIndex, _indents[indentLevel]); + insertedIndent += _indents[indentLevel].Length; + } + + foreach (var i in scope.BreakPositions) + { + _sb.Insert(i + insertedIndent, _indents[indentLevel]); + insertedIndent += _indents[indentLevel].Length; + } + + if (indentLevel != 0) + AppendLine(indentLevel - 1); + } + + private void CheckForScope() { - _indentLevel--; - AppendLine(); + if (_scopes.Count == 0) + throw new InvalidOperationException($"Must start a scope with {nameof(StartScope)}()"); } + public record struct Scope(int StartIndex) + { + public List BreakPositions { get; } = new (); + }; + internal class DisposableIndent : IDisposable { private readonly IndentedStringBuilder _isb; @@ -325,7 +363,7 @@ public void Dispose() if (_disposed) return; - _isb.DecreaseIndent(); + _isb.EndScope(); _disposed = true; } } @@ -335,18 +373,19 @@ public record PrinterOptions( int IndentCount, char IndentChar, bool BreakOnParenExpr, - bool BreakOnBinaryOperators + bool BreakOnBinaryOperators, + int BreakAfterCharCount ) { /// /// Formats the printed output of PromQL expressions into a more human-readable format, /// by inserting line breaks and indentation. /// - public static PrinterOptions PrettyDefault = new(2, ' ', true, true); + public static PrinterOptions PrettyDefault = new(2, ' ', true, true, 100); /// /// Doesn't format the printed output of PromQL expressions. /// - public static PrinterOptions NoFormatting = new(0, ' ', false, false); + public static PrinterOptions NoFormatting = new(0, ' ', false, false, int.MaxValue); } } \ No newline at end of file diff --git a/src/PromQL.Parser/PromQL.Parser.csproj b/src/PromQL.Parser/PromQL.Parser.csproj index 856b146..c1f7d75 100644 --- a/src/PromQL.Parser/PromQL.Parser.csproj +++ b/src/PromQL.Parser/PromQL.Parser.csproj @@ -2,7 +2,7 @@ netcoreapp3.1;net6.0 - 9 + 10 enable true PromQL.Parser diff --git a/tests/PromQL.Parser.Tests/PrinterTests.cs b/tests/PromQL.Parser.Tests/PrinterTests.cs index 40e5e30..1bcd115 100644 --- a/tests/PromQL.Parser.Tests/PrinterTests.cs +++ b/tests/PromQL.Parser.Tests/PrinterTests.cs @@ -214,30 +214,74 @@ public class PrettyPrintTests private Printer _printer = new Printer(PrinterOptions.PrettyDefault); [Test] - public void ParenExpression() => _printer.ToPromQl(new ParenExpression(new NumberLiteral(1.0))) - .Should().Be(@"( - 1 + public void ParenExpressionShort() => _printer.ToPromQl(new ParenExpression(new NumberLiteral(1.0))) + .Should().Be(@"(1)"); + + [Test] + public void ParenExpressionNestedShort() => _printer.ToPromQl(new ParenExpression(new ParenExpression(new NumberLiteral(1.0)))) + .Should().Be(@"((1))"); + + [Test] + public void BinaryExpressionShort() => _printer.ToPromQl(new BinaryExpr(new NumberLiteral(1.0), new NumberLiteral(1.0), Operators.Binary.Add)) + .Should().Be(@"1 + 1"); + + [Test] + public void ParenBinaryExpressionShort() => _printer.ToPromQl(new ParenExpression(new BinaryExpr(new NumberLiteral(1.0), new NumberLiteral(1.0), Operators.Binary.Add))) + .Should().Be(@"(1 + 1)"); + + [Test] + public void BinaryExpression() => _printer.ToPromQl(Parser.ParseExpression("sum(rate(this_is_a_long_metric_name{environment='production'}[5m])) + sum(rate(this_is_another_long_metric_name{environment='production'}[5m]))")) + .Should().Be( +@"sum(rate(this_is_a_long_metric_name{environment='production'}[5m])) ++ sum(rate(this_is_another_long_metric_name{environment='production'}[5m]))"); + + [Test] + public void BinaryWrappedParenExpression() => _printer.ToPromQl(Parser.ParseExpression("(sum(rate(this_is_a_long_metric_name{environment='production'}[5m])) + sum(rate(this_is_another_long_metric_name{environment='production'}[5m])))")) + .Should().Be( + @"( + sum(rate(this_is_a_long_metric_name{environment='production'}[5m])) + + sum(rate(this_is_another_long_metric_name{environment='production'}[5m])) )"); [Test] - public void ParenExpressionNested() => _printer.ToPromQl(new ParenExpression(new ParenExpression(new NumberLiteral(1.0)))) - .Should().Be(@"( + public void BinaryWrappedParenExpression2() => _printer.ToPromQl(Parser.ParseExpression("((sum(rate(this_is_a_long_metric_name{environment='production'}[5m])) + sum(rate(this_is_another_long_metric_name{environment='production'}[5m]))))")) + .Should().Be( + @"( ( - 1 + sum(rate(this_is_a_long_metric_name{environment='production'}[5m])) + + sum(rate(this_is_another_long_metric_name{environment='production'}[5m])) ) )"); [Test] - public void BinaryExpression() => _printer.ToPromQl(new BinaryExpr(new NumberLiteral(1.0), new NumberLiteral(1.0), Operators.Binary.Add)) - .Should().Be(@"1 -+ 1"); + public void BinaryExpression2() => _printer.ToPromQl(Parser.ParseExpression("1 + 2 + 3 + 4 + 5 + sum(rate(this_is_another_long_metric_name{environment='production', code=~'5.*'}[5m]))")) + .Should().Be( +@"1 ++ 2 ++ 3 ++ 4 ++ 5 ++ sum(rate(this_is_another_long_metric_name{environment='production', code=~'5.*'}[5m]))"); [Test] - public void ParenBinaryExpression() => _printer.ToPromQl(new ParenExpression(new BinaryExpr(new NumberLiteral(1.0), new NumberLiteral(1.0), Operators.Binary.Add))) + public void BinaryExpressionNestedParenShort() => _printer.ToPromQl(Parser.ParseExpression("1 + 2 + (3 + (4 + 5))")) + .Should().Be( + @"1 + 2 + (3 + (4 + 5))"); + + [Test] + public void BinaryExpressionNestedParenLong() => _printer.ToPromQl(Parser.ParseExpression("1 + (2 + 3) + sum(rate(this_is_another_long_metric_name{environment='production', code=~'5.*', endpoint='/api/test'}[5m]))")) + .Should().Be( +@"1 ++ (2 + 3) ++ sum(rate(this_is_another_long_metric_name{environment='production', code=~'5.*', endpoint='/api/test'}[5m]))"); + + + [Test] + public void ParenBinaryExpression() => _printer.ToPromQl(Parser.ParseExpression("(sum(rate(this_is_a_long_metric_name{environment='production'}[5m])) + sum(rate(this_is_another_long_metric_name{environment='production'}[5m])))")) .Should().Be(@"( - 1 - + 1 + sum(rate(this_is_a_long_metric_name{environment='production'}[5m])) + + sum(rate(this_is_another_long_metric_name{environment='production'}[5m])) )"); - + } } \ No newline at end of file