-
-
Notifications
You must be signed in to change notification settings - Fork 294
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
add tune type in list #989
base: main
Are you sure you want to change the base?
Conversation
Thanks for this. That's a useful feature. The changes to I don't think the "get" function should change the object - it should be read only. So the return of Also, you are saving the key. Should there be a parallel function to retrieve tunes by key? Or perhaps even a combination of type and key? And for PRs in general, I make the changes to the |
Hi Paul,I’m not sure how a pull request occurred am sorry about that. I planned to create one when I had time to finish the feature. I’m glad you like it. Thank you for your feedback. I’m not sure when I will get the chance finish it so feel free to make the changes. Great library by the way, very useful.Regards,Meriadoc Lawes.On 10 Apr 2024, at 18:47, Paul Rosen ***@***.***> wrote:
Thanks for this. That's a useful feature. The changes to abc_parse_book.js look fine, but I think I'd like to change some things in abc_tunebook.js.
I don't think the "get" function should change the object - it should be read only. So the return of getTunesByType should be the result of the filter and this.tunes should be unchanged.
Also, you are saving the key. Should there be a parallel function to retrieve tunes by key? Or perhaps even a combination of type and key?
And for PRs in general, I make the changes to the dev branch and this wants to merge to main. Otherwise I'd accept the pull request then modify it myself.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
No description provided.