zmoon / PyABC2

ABC music notation tools
MIT License
5 stars 1 forks source link

Conversion to and from Helmholtz #42

Closed mccalluc closed 1 year ago

mccalluc commented 1 year ago

Hope this is on the right track, but please comment if not.

In Pitch.from_name, _class_name and _octave are being set on the new object: Is this something we should do in every case?

The logic overlaps with _octave_from_abc_parts: Easiest just to leave it as-is, but another possibility would be to use the new from_helmholtz here.

mccalluc commented 1 year ago

That all makes sense. I had held off on a tighter regex because it felt like the concerns were overlapping, but you're right, having a clear exception thrown hear is better than getting a cryptic one from deeper in the code.

I'll make this a draft, tweak, push, and re-request review. Thanks!

codecov-commenter commented 1 year ago

Codecov Report

Merging #42 (026022c) into main (e71bb38) will increase coverage by 0.24%. The diff coverage is 95.83%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
+ Coverage   89.15%   89.40%   +0.24%     
==========================================
  Files           9        9              
  Lines        1300     1321      +21     
==========================================
+ Hits         1159     1181      +22     
+ Misses        141      140       -1     
Impacted Files Coverage Δ
pyabc2/pitch.py 86.05% <95.83%> (+0.89%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.