Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(scale_pattern): ✨ add double harmonic major scale #482

Conversation

JulioJamon54
Copy link
Contributor

  • feat(scale_pattern): ✨ add double harmonic major scale

  • test(scale_pattern): 🧪 add test cases for double harmonic major scale

With documentation, Wikipedia diagram, and unit tests for the scale notes, name and toString()
Copy link
Owner

@albertms10 albertms10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for your quick and efficient contribution!

Just one aspect: is there any Double harmonic minor scale at all? If so, adding it to this PR would be great, too! 😸

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8759271536

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 8730428655: 0.0%
Covered Lines: 1158
Relevant Lines: 1158

💛 - Coveralls

@JulioJamon54
Copy link
Contributor Author

Thank you!
There is a double harmonic minor scale, but I think it is most commonly called hungarian minor scale:
https://en.wikipedia.org/wiki/Hungarian_minor_scale
Do you want me to add it anyway? 😄

@albertms10
Copy link
Owner

albertms10 commented Apr 21, 2024

Great, thanks for the research! (and apologies for the ignorance)

It seems curious at first that the Double harmonic minor scale is also called Hungarian minor, but the Hungarian major scale is not the same as the former Double harmonic major scale. 😅—Please correct me if I’m wrong!

Given this naming convention is more complex than I expected, I would just add the Double harmonic major scale, sticking to the initial PR goal. Sorry for the noise!

@albertms10 albertms10 merged commit 878d581 into albertms10:main Apr 21, 2024
3 checks passed
@albertms10 albertms10 added this to the Road to 0.18 milestone May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants