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

Allow common music notation chord names #50

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

enjikaka
Copy link

This rewrites the specification of chords to allow for more names than the constant. For example: maj7, dim etc. This is to easier extend with more special chords later like: 7b9, dim7, 7s9 etc without having awkwardly long constant names.

@enjikaka enjikaka mentioned this pull request Jan 14, 2019
@@ -20,9 +19,15 @@
"engines": {
"node": ">=8"
},
"keywords": ["music", "utility", "audio"],
Copy link
Author

@enjikaka enjikaka Jan 14, 2019

Choose a reason for hiding this comment

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

I think Prettier went wild here and changed the formatting. 🙈

@geoffreydhuyvetters
Copy link
Member

Thanks for the PR :)

I think it might make sense to use the Airbnb code style guide here.

CHORD.minorMajor instead of Chord.MINOR_MAJOR etc...

What do you think @enjikaka?

@geoffreydhuyvetters
Copy link
Member

@enjikaka
Copy link
Author

enjikaka commented Jan 14, 2019

I don't feel strongly for the casing differences.minorMajor feels closer to normal music notation; in like in maj7, add9. (If you regard the numer as the next part in camelCase). However, constants are normally WRITTEN_LIKE_THIS in JavaScript, not the place that gathers the constants (Chord in this case).

So I'm not sure what is best in this case. 🤔 Would a lot of stuff break if we changed to minorMajor?

@geoffreydhuyvetters
Copy link
Member

I see your point, that would probably also mean we would have to change the intervals / scales etc for consistency.

I wasn't thinking about the case where you are just importing a specific chord type.

@geoffreydhuyvetters
Copy link
Member

Had another thought about it, and how would you feel if we did it a bit more explicit like Interval for now. (not generating the object and spreading it). To maximize tree shaking and splitting all code should be statically analyzable.

I would love to look into using some codegen method to create the export files before build.

Looked into this but never got it to work 😞

Does that make sense? I really appreciate your efforts.

@enjikaka
Copy link
Author

@duivvv I changed to that now! 👍 Did notice there are 4 flow errors related to intervals though. But maybe you're on to them?

@geoffreydhuyvetters
Copy link
Member

I'm trying something out with babel-plugin-codegen in a different branch, will tag you there when I open the PR so we can discuss it. I might be on to something that would make it easy to maintain, allow for static exports and make it easy to add aliases (what we're trying to achieve in this PR I guess)

@geoffreydhuyvetters
Copy link
Member

It would have the same result as above + the advantage of having all chords centralized in 1 file.

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.

2 participants