What's wrong
Keybinding/Services/KeybindingService.cs:178-183:
public Phrase ParsePhrase(string phraseString)
{
if (string.IsNullOrWhiteSpace(phraseString))
{
return new Phrase([]);
}The Phrase constructor (MusicalTypes.cs:~318) rejects an empty sequence, so this branch can never return. Every call with null, "" or whitespace throws:
ArgumentException: Phrase must contain at least one chord (Parameter 'sequence')
at Phrase..ctor
at KeybindingService.ParsePhrase KeybindingService.cs:182
Why it matters
The guard was clearly written to handle empty input in a controlled way, but the exception it produces names a parameter (sequence) that the caller never passed. A settings UI that parses a user's cleared keybinding field gets a confusing error that points inside the library. ParseChord validates its argument and throws with nameof(chordString), so the two parse methods behave differently.
Suggested fix
Pick one contract and implement it:
- Throw
new ArgumentException("Phrase string cannot be empty.", nameof(phraseString)), as ParseChord does. This is the minimal fix and doesn't change the API.
- Or change the method to return
Phrase? (or add TryParsePhrase) and return null for empty input.
Acceptance: ParsePhrase("") no longer throws from Phrase..ctor, and a test pins the chosen behaviour.
What's wrong
Keybinding/Services/KeybindingService.cs:178-183:The
Phraseconstructor (MusicalTypes.cs:~318) rejects an empty sequence, so this branch can never return. Every call withnull,""or whitespace throws:Why it matters
The guard was clearly written to handle empty input in a controlled way, but the exception it produces names a parameter (
sequence) that the caller never passed. A settings UI that parses a user's cleared keybinding field gets a confusing error that points inside the library.ParseChordvalidates its argument and throws withnameof(chordString), so the two parse methods behave differently.Suggested fix
Pick one contract and implement it:
new ArgumentException("Phrase string cannot be empty.", nameof(phraseString)), asParseChorddoes. This is the minimal fix and doesn't change the API.Phrase?(or addTryParsePhrase) and returnnullfor empty input.Acceptance:
ParsePhrase("")no longer throws fromPhrase..ctor, and a test pins the chosen behaviour.