search

paulrosen / abcjs

javascript for rendering abc music notation
Other
1.94k stars 285 forks source link

add tune type in list #989

Open meriadocl opened 9 months ago

paulrosen commented 7 months ago

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.

meriadocl commented 7 months ago

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: @.***>