💡 Refactor Chord parsing

[martijnversluis]

The current API for parsing and formatting chords is:

• inconsistent with how parsing and formatting chord sheets works
• poorly designed, as adding support for numerals with add an additional class and additional branching inside the parsing and formatting functions

Compare the following APIs:

Use case	Parsing	Formatting	Converting
Chord sheet	const song = new ParserClass(options).parse(chordSheet);	new FormatterClass(options).format(song);
Chord (previously)	const chord = Chord.parse(chordString);	chord.toString();
Chord (currently)	const chord = parseChord(chordString);	chord.toString();	toChordSymbol(chord)
Chord (idea 1)	const chord = new ChordParser(options).parse(chordString);	new ChordSymbolFormatter(options).format(chord);	new ChordSymbolFormatter(options).format(chord); new ChordNumericFormatter(options).format(chord);
Chord (idea 2)	const chord = Chord.parse(chordString);	chord.toString();	chord.toChordSymbolString(); chord.toNumericString();

[isaiahdahl]

Yea I'm onboard for a refactor to put ourselves in a better spot for the long game.

What sort of options do you think would be passed in to the ChordParser with Option 1?

At first glance I liked idea 2 better because it seemed to feel more comfortable to me and read easier. ie. chord.toString(); just seems so much cleaner than new ChordSymbolFormatter(options).format(chord);

But I could also see the pros to being able to pass options to the ChordSymbolFormatter. Might give us more flexibility.

In converting would we need a ChordSymbolFormatter and a ChordNumbericFormatter which would mean we'd also need a ChordNumeralsFormatter when we get there, and possibly in future ChordDoReMiFormatter (not in current scope but it is a possibility) Would it be an idea for Numbers, Numerals, Chords etc to just be an option passed into the ChordSymbolFormatter?

[martijnversluis]

What sort of options do you think would be passed in to the ChordParser with Option 1?

None so far, it rather showed that this kind of API would allow for passing options.

I must say I like option 2 better, the only disadvantage is that it isn't consistent with the chord sheet API (not sure if that's a problem that must be solved).

I'd imagine having just one Chord class. When parsing, it will at least store a "note number". For numbers, numerals and doremi that's simple, when parsing a chord symbol the number is determined relative to the song key. This allows for converting and formatting the chord to any other type., using functions like toChordSymbol(), toNumber(), toNumeral() and toDoReMi()

[isaiahdahl]

So where do we plan to handle the song key with regards to normalizing and exlcuisions https://github.com/PraiseCharts/ChordChartJS/issues/16?

Maybe that is an example of something passed into the ChordParser(options)?

I think the song key has to be passed down to the chord class in some form because it determines different logic for the exclusions, which should be handled when you normalize. So maybe a combination?

options = {
     songKey: "A",
     ignoreExclusions: false, // ??
}

const chord = new ChordParser(options).parse(chordString);

chord.toString();
chord.toNumber();
// etc ...

Thoughts?

I definitely like the simplicity of option 2 for formatting and converting and I'm not sure if the inconsistency is really that much of a downside. They serve different purposes. But open to thoughts.

[martijnversluis]

Yeah, I agree on the simplicity of option 2.

I'm not sure when/where to do the normalisation. I would imagine the parsing to be as lossless as possible, and perform normalisation etc using method calls on the chord object.

Maybe I'll try to come up with something that passes the tests and seems reasonable, and you can see if it makes sense for you?

[isaiahdahl]

Sounds good. Ultimately you make the final call. I'll continue to add my thoughts to hopefully at least bring another angle to things. But you have the final say on how we implement as long as it satisfies the 'objectives'