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 ns meta to a handful of clojure.core dynamic vars #597

Merged
merged 4 commits into from Aug 5, 2021

Conversation

bobisageek
Copy link
Contributor

Because the vars are already marked dynamic, we can't just use core-var (without adding more arguments or something), so I added the function for just tweaking the metadata on the dynamic vars. As always, I'm happy to revise it if there's a different preferred approach.

@borkdude
Copy link
Collaborator

borkdude commented Aug 4, 2021

I think we could just add this metadata at the place where these vars are first created.

@bobisageek
Copy link
Contributor Author

I thought about that, but it didn't work out in my head, so I'm hoping you can point out where I'm tripped up on this.

We can't pull the namespace object from namespaces into vars and io because it would create a cyclic dependency. We could create a namespace object in the locations where those vars are created, but then the namespace objects across core wouldn't be the exact same namespace object, which feels undesirable.

We could create the namespace just once in vars and refer to it from io and namespaces, but that felt like it would slightly dirty up the vars namespace a bit. With that said, that's where the user ns is created, so maybe the "dirty-ness" is just me being very particular. Is this approach (define the core namespace in vars and re-use it elsewhere) what you have in mind?

@borkdude
Copy link
Collaborator

borkdude commented Aug 4, 2021

@bobisageek We can put these ns objects in sci.impl.utils.

@borkdude
Copy link
Collaborator

borkdude commented Aug 4, 2021

O wait, that doesn't work because of the cyclic dep.
We can put those in sci.impl.types then... Or vars.

@borkdude
Copy link
Collaborator

borkdude commented Aug 4, 2021

Yeah, let's do sci.impl.vars. To be fair, that namespace is also about namespaces, not only vars. So it seems fine.

@bobisageek
Copy link
Contributor Author

Awesome. I'll see if I can make some headway on this after work today.

- create clojure core ns in sci.impl.vars
- refer to vars ns in io and namespaces
- "alias" clojure-core-ns in namespaces to maintain babashka.main's reference (for now)
@bobisageek
Copy link
Contributor Author

This may seem a little bit convoluted, but I left the clojure-core-ns symbol in namespaces to keep babashka whole (for now). Since b.impl.clojure.core refers to clojure-core-ns from namespaces, and I wanted to keep the pull requests separate (between babashka and sci), I left the symbol in both namespaces, and then the plan is to change the :refer in b.impl.clojure.core, at which point it will be safe to remove the symbol from s.impl.namespaces.

I sincerely hope I'm not being more annoyance than help. :)

@borkdude borkdude merged commit 4123d58 into babashka:master Aug 5, 2021
@borkdude
Copy link
Collaborator

borkdude commented Aug 5, 2021

I sincerely hope I'm not being more annoyance than help. :)

I sincerely think you're being very helpful!

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.

None yet

2 participants