Skip to content

Section.Parse and Arrangement.Parse silently drop every progression line after the first: a two-line verse loses its second line, and trailing garbage is accepted #437

Closed

Description

@matt-edmondson

What's wrong

Section.TryParse splits the text into lines, reads the header from lines[0] and the progression from lines[1], and never looks at lines[2..]:

string[] lines = text.Replace("\r", string.Empty).Split('\n');
if (lines.Length < 2 || lines[0].Length == 0 || lines[0][0] != '[')

if (!Progression.TryParse(lines[1], out Progression? progression))
{
return false;
}
result = Create(type, progression, label, key);
return true;

Arrangement.TryParse hands each blank-line-separated block to Section.TryParse, so it inherits the same behaviour:

string[] blocks = text.Replace("\r", string.Empty).Split(["\n\n"], System.StringSplitOptions.RemoveEmptyEntries);
if (blocks.Length < 2 || !Key.TryParse(blocks[0].Trim(), out Key? key))
{
return false;
}
List<Section> sections = [];
for (int i = 1; i < blocks.Length; i++)
{
if (!Section.TryParse(blocks[i], out Section? section))
{
return false;
}
sections.Add(section);
}

Anything after the first progression line is thrown away, and parsing still reports success.

Reproduction (net10.0, current main)

Section s = Section.Parse("[Verse]\n4/4 C | G |\nAm | F |");
// s.Progression.Chords.Count == 2  (C, G). Am and F are gone.
// s.ToString() == "[Verse]\n4/4  C G"

Section.TryParse("[Verse]\n4/4 C | G |\nthis is garbage", out Section? g);
// returns true

Arrangement a = Arrangement.Parse("C major\n\n[Verse]\n4/4 C | G |\nAm | F |\n\n[Chorus]\n4/4 F | G |");
// Verse has 2 chords, not 4

Why it matters

Chord charts are usually written several bars to a line, so a verse that spans two or more lines is the normal case. A caller who parses one gets a shorter progression back. Because nothing fails, the missing bars carry through into Bars, Form.Of, cadence detection and key inference without any warning. The parser also accepts input it does not understand, which goes against the strict TryParse contract the rest of the Music types follow (#318 and #411 tightened the header for the same reason).

Suggested fix / acceptance criteria

Pick one of these and pin it with tests:

  • Reject: TryParse returns false (and Parse throws FormatException) when a non-blank line follows the progression line. Trailing empty lines, such as the newline at the end of a file, are still accepted.
  • Or support multi-line sections: join the continuation lines into one progression, treating a line break as a bar line, so the verse above parses as C G Am F. ToString keeps writing one line, so the round trip still holds.

Either way, no input should parse successfully while part of its progression is dropped. Add tests at both the Section and the Arrangement level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions