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

add user script directory to search paths of 'require' #62

Merged
merged 2 commits into from
May 15, 2022

Conversation

bobisageek
Copy link
Contributor

fixes #38

With this change, (require) checks the workspace scripts directory first, then the user script directory, then fails. It incidentally also adds a nil check on the workspace root before trying to load it, so in cases where Joyride currently complains about a null when requiring a script with no workspace loaded, this prevents the call to path/join if there's no workspace root. Finally, if there's no matching file in the workspace or user scripts directory, Joyride currently presents a JS-ish error message (ENOENT). This change returns nil from sci's :load-fn if no matching files are found, resulting in the message Could not find namespace: missing-ns.

It does still use the synchronous fs stuff for now.

src/main/joyride/sci.cljs Outdated Show resolved Hide resolved
Copy link
Collaborator

@PEZ PEZ left a comment

Choose a reason for hiding this comment

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

Looks awesome to me. I have tested that the example I mention in #38 works as well.

@PEZ PEZ requested a review from borkdude May 15, 2022 19:46
@borkdude
Copy link
Collaborator

LGTM!

@bobisageek
Copy link
Contributor Author

I'm going to tweak the config alias and re-run my local tests (because I'm always paranoid about the 'quick changes').

@PEZ PEZ merged commit c97d596 into BetterThanTomorrow:master May 15, 2022
@PEZ
Copy link
Collaborator

PEZ commented May 15, 2022

Thanks, @bobisageek! 🙏

@bobisageek bobisageek deleted the require-ws-or-user-script branch May 15, 2022 21:42
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.

(require) does not load ns from user script dir
3 participants