search

finale-lua / lua-scripts

A central repository for all Lua scripts for Finale.
https://finalelua.com
Creative Commons Zero v1.0 Universal
15 stars 14 forks source link

lint fixes and build broken #646

Closed rpatters1 closed 8 months ago

rpatters1 commented 8 months ago

page_singles_changer.lua broke the build because of the raw call to require for the embedded cjson c-library. Recall that the build parses the source files for dependencies by looking for require statements. Therefore, in this repo we have to wrap those require statements for embedded utilities with a call to util.require_embedded.

While I was at it, I fixed a couple of lint errors and changed the code to use library.calc_script_name instead of hard-coding the script name.

Please test these changes. If you would rather hard-code the script name, that's fine. I just like to avoid hard-coding when possible.

asherber commented 8 months ago

@rpatters1 Maybe the docs should be updated regarding requiring embedded libs? On your site and the finalelua site it still says just to do require('cjson'). Maybe the explanation is that you can do this in your own personal scripts, but you should use require_embedded for any script submitted to the repo.

rpatters1 commented 8 months ago

As issue with the docs is that the RGP Lua docs are targeted at any RGP Lua user whereas the need to use utils.require_embedded is just an idiosyncrasy of this repo. I suppose this could be spelled out in the docs.

asherber commented 8 months ago

Yes, that's what I was trying to get at in my comment.

cv-on-hub commented 8 months ago

PS: hadn't noticed library.calc_script_name() before. Will use it universally now instead of hard coding.

jwink75 commented 8 months ago

Can I get confirmation that going forward (and maybe even on older scripts) we are all supposed to use utils.require_embedded(“…..”) rather than require(“…..”)?I’ll also check out library.calc_script_name()  …Sent from my iPhoneOn Jan 20, 2024, at 1:15 PM, Carl Vine @.***> wrote: @cv-on-hub requested changes on this pull request.

That all looks terrific except for line 32 in pitch_singles_changer.lua. "library" is misspelt as "libary". I had previously used utils.require_embedded("cjson") but changed to require('cjson') because of the documentation. Happy to revert. Thanks also for converting globabl_info into a mixin dialog variable.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

rpatters1 commented 8 months ago

You only use utils.require_embedded for C libraries embedded in RGP Lua. I believe there are three (possibly 4 if you count socket) that require this. (cjson, lfs, and luaosutils). I could conceivably embed more, but I've looked for widely used C libraries that have utility in Finale, and I am hard-pressed to find more. Maybe LPeg? (It's powerful but looks like it has a steep learning curve.)

All the regular lua libraries should continue to use require.

The reason we need utils.require_embedded is that the build parses the files for require statements and dumps the libraries into the dist version of the script, so that we can keep the distributed scripts free of dependencies. But since the embedded C libraries aren't Lua and aren't files, the build throws up its hands and quits unless you disguise the require statement so that the build ignores it.

jwink75 commented 8 months ago

OK, thanks, that makes sense.

From a usage standpoint, would I do something like:

local osutlities = utils.require_embedded("luaosutils”)

?

On Jan 20, 2024, at 4:41 PM, rpatters1 @.***> wrote:

You only use utils.require_embedded for C libraries embedded in RGP Lua. I believe there are three (possibly 4 if you count socket) that require this. (cjson, lfs, and luaosutils). I could conceivably embed more, but I've looked for widely used C libraries that have utility in Finale, and I am hard-pressed to find more. Maybe LPeg https://www.inf.puc-rio.br/~roberto/lpeg/? (It's powerful but looks like it has a steep learning curve.)

All the regular lua libraries should continue to use require.

The reason we need utils.require_embedded is that the build parses the files for require statements and dumps the libraries into the dist version of the script, so that we can keep the distributed scripts free of dependencies.

— Reply to this email directly, view it on GitHub https://github.com/finale-lua/lua-scripts/pull/646#issuecomment-1902463893, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMCBUQYCJXDLL3YCUEHOD6LYPRP3VAVCNFSM6AAAAABCDGG5NSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBSGQ3DGOBZGM. You are receiving this because you commented.

asherber commented 8 months ago

@cv-on-hub I wanted to call out that the build process has been revised so that require_embedded() is no longer needed (in fact, is no longer available). Your current scripts have been adjusted to just use require(), and you should use this going forward. We've also updated the docs to reflect this.

cv-on-hub commented 8 months ago

call out that the build process has been revised

Thanks again @asherber, and for updating hotkey_script_palettes.lua. Another of my scripts uses cjson (pitch_singles_changer.lua) which I've updated on a new PR.