switched chord label validation to regexp method #242
Conversation
Added test case for chord regexp
There was a problem hiding this comment.
Thanks. I assume chord experts (@ejhumphrey @mattmcvicar or others) have given this a thorough run-through in JAMS?
| # which is in turn derived from the context-free grammar of | ||
| # Harte et al., 2005. | ||
|
|
||
| pattern = re.compile(r'''^((N|X)|(([A-G](b*|#*))((:(maj|min|dim|aug|1|5|sus2|sus4|maj6|min6|7|maj7|min7|dim7|hdim7|minmaj7|aug7|9|maj9|min9|11|maj11|min11|13|maj13|min13)(\((\*?((b*|#*)([1-9]|1[0-3]?))(,\*?((b*|#*)([1-9]|1[0-3]?)))*)\))?)|(:\((\*?((b*|#*)([1-9]|1[0-3]?))(,\*?((b*|#*)([1-9]|1[0-3]?)))*)\)))?((/((b*|#*)([1-9]|1[0-3]?)))?)?))$''') # nopep8 |
There was a problem hiding this comment.
Why use a triple quote if it's only one line?
Splitting into a few lines would not be so bad either.
There was a problem hiding this comment.
Splitting the line also prevents GitHub from doing nasty scrolling things.
There was a problem hiding this comment.
Splitting a regexp into multiple lines is inviting trouble, IMO. The triple-quotes aren't strictly necessary, but they don't hurt either.
There was a problem hiding this comment.
Splitting a regexp into multiple lines is inviting trouble, IMO.
I dunno, wcgw? And you can even add comments for each part of the otherwise inscrutable regex:
https://github.com/craffel/pretty-midi/blob/master/pretty_midi/utilities.py#L82
The triple-quotes aren't strictly necessary, but they don't hurt either.
Isn't this true of literally every string in Python?
There was a problem hiding this comment.
I dunno, wcgw?
I generally dislike relying on invisible string concatenation because it's brittle. If, somehow, the string no longer exists inside parentheses, say because you pull it out to its own variable name exp = '....' then it fails silently. I've been burned by this kind of thing before, so I try to avoid it.
Isn't this true of literally every string in Python?
Not strings that could have apostrophes in them (which we don't in this case) or could be interpolated via %/{} (which we also don't). Maybe it's just my own quirk to use r''' ''' for all regexps just in case those things do pop up naturally in the expression.
There was a problem hiding this comment.
if, somehow, the string no longer exists inside parentheses,
You could make this argument for any expression in Python which appears in parentheses.
I've been burned by this kind of thing before, so I try to avoid it.
I sense some PTSD. We can just leave it as-is.
Yup. It's exactly the grammar from Harte's paper (which is right-recursive and hence regular, hurray for the chomsky language hierarchy!), with additional terminals for |
This fixes #217 by using a regexp to parse chords, rather than explicit case splitting.
The regexp is ported over from JAMS, where it has already been thoroughly tested. I added an extra test case 'asdf' that was mentioned in the #217 thread as previously passing.
Because the regexp is such a monster, I disabled the pep8 length check for the corresponding line.