ozankasikci / rust-music-theory

A music theory guide written in Rust.
MIT License
626 stars 28 forks source link

re: Change encoding of pitch classes #20

Closed henryksloan closed 4 years ago

henryksloan commented 4 years ago

Original PR is #15 by @jagen31

On top of #15, this PR:

Changes from master

Some notes going forward (pun intended)

@ozankasikci tests run perfectly, but the actual test code and style consistency could use a good lookover

jagen31 commented 4 years ago

I came here to finally take care of this just now and it's already been done! Thanks!

ozankasikci commented 4 years ago

That's super cool @henryksloan!

Left some comments for the pitch function and an arbitrary number of accidentals support.

Let me know what you think! 💪

henryksloan commented 4 years ago

@ozankasikci good points, all done! I removed that pitch function. As for multiple accidentals, I added support for an arbitrary number of accidentals of the same type, i.e. C##x returns Some(Pitch {letter: C, accidental: 4}), because sharp+sharp+double sharp = 4. That kind of thing is not uncommon when representing higher-order accidentals. I return None when flats and sharps are combined, like in C#b.

I also extended REGEX_PITCH to allow multiple accidentals. The regex now accepts any number of accidents. I think it would be unnecessarily complex to encode the no-mixing constraint into a regex, especially since Rust's built-in regexes don't support backreferencing.