Support requireing modules located within the script folder
#37
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…pport the use of "require" for scripts in the script folder.
Enable scripts_<etc> = "filename.lua" Reduces the noise around script definitions, helps with supporting the disablement of "greedy loading" of "open" scripts.
…ipts_window_open' 'scripts_window_open' being defined disables "greedy loading" to enable 'require' in user scripts. Defined is: * A table with filename entries (as other script defs). * An empty table definition. * A single string, empty or not.
...and the call to g_build_path, since the same stuff was being done in a few places. Also followed clangs advice and added some const to args.
Re-arrange slightly and modify to explain: * Changes to support single file definition * scripts_window_open addition and significance * Explain "require"ing modules re: the previous/default behaviour.
...and more consistent/predictable for other files and the config. The Lua VM is re-loaded if a module or any other file has changed, or if the config does, as any changes can render any global variables or state invalid.
Safe to just wait for the next edit and change event to load a valid config
dsalt
reviewed
Jan 20, 2025
src/config.c
Outdated
Comment on lines
236
to
238
| script_folder, | ||
| current_file, | ||
| NULL); |
There was a problem hiding this comment.
Indent with tabs, align with spaces; and if your text editor ever replaces the spaces with tabs, it's broken because doing so will break the alignment for others who use a different tab width.
dsalt
reviewed
Jan 20, 2025
src/config.c
Outdated
Comment on lines
137
to
139
| script_folder, | ||
| oneFileString, | ||
| NULL); |
There was a problem hiding this comment.
3 tabs to indent then 36 spaces (not 9 tabs == 72 spaces) to align.
There was a problem hiding this comment.
This was fixed/changed in a later commit, which was part of the original push: 2e711f8#diff-18680f57f5d32607a3b196afffe0d6ecb250af6c0a994b304b88fc91231250bfR135
Remove unneeded indentaion left over from placement in inner loop, fix formatting of assignment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was intended to be a fairly small PR, to include the script folder in Lua's package search path, ended up getting a little more involved:
This PR resolves this by:
scripts_window_openvariable.scripts...variables to be assigned a single string, referencing a single file.In support of the above changes I also:
add_lua_file_to_listto accept the script folder path and include the call tog_build_pathsince the same thing was being done in a few places.refresh_config_and_script).load_config.Why not other implementations?
scripts_window_open, anyway, and have 2 things to add to config when 1 would do the same job.script_...tables. Would also require more processing to end up with, basically, the same "window open" list anyway. Also begs a question of would it only apply to the "open" list, or all?@dsalt This is one of a total of 5 PRs so I'm sorry for the github spam!
If you decide to accept them and would rather I group them into 1 larger PR, just let me know.
Thanks!