Skip to content

Conversation

@filipesilva
Copy link
Contributor

@filipesilva filipesilva commented Sep 14, 2025

Should be working, with the notable exceptions of :clj-reload/keep metadata in deftype/defrecord/defprotocol.

I traced it down to how clj-reload.keep/patch-file will create an import like (clojure.core/import clj_reload.keep_deftype.TypeKeep) in keep-type-test, but in Babashka that's not available anymore after clj-reload.core/ns-unload calls (remove-ns ns). So in bb we get java.lang.Exception: Unable to resolve classname: clj_reload.keep_deftype.TypeKeep.

I'm guessing Clojure's remove-ns doesn't remove the Java types but in Babashka it does.

cc @borkdude

"namespace '%s' not found after loading '%s'"
ns (.getPath file))
(when (not (find-ns ns))
(throw (Exception. (format "namespace '%s' not found after loading '%s'" ns (.getPath file)))))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Babashka doesn't have this private fn.

Not the same exception, but it didn't seem significant for this to be a compiler exception with line and column.


(defn meta= [a b]
(= (dissoc (meta a) :ns) (dissoc (meta b) :ns)))
(= (dissoc (meta a) :ns :file) (dissoc (meta b) :ns :file)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In bb :file goes from absolute to relative here:

(-> a meta :file) /Users/filipesilva/repos/sandbox/clj-reload/fixtures/keep_test/clj_reload/keep_vars.clj
(-> b meta :file) clj_reload/keep_vars.clj

Didn't seem important to check. I think this test just cares about the user metadata being retained.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually I would like to keep the check here. Happy to skip it for bb? only and leave it for Clojure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, done and pushed 👍

@filipesilva filipesilva force-pushed the support-bb branch 2 times, most recently from f160981 to a03e8aa Compare September 14, 2025 15:50
(is (not (identical? keep-new @(ns-resolve ns' 'type-keep-new))))
(is (= keep-new @(ns-resolve ns' 'type-keep-new)))
(is (identical? (class keep-new) (class @(ns-resolve ns' 'type-keep-new))))
(when-not (tu/babashka?)
Copy link
Contributor Author

@filipesilva filipesilva Sep 14, 2025

Choose a reason for hiding this comment

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

Compare with github's "Hide whitespace". I'm just not running these 4 tests when on bb.

@filipesilva
Copy link
Contributor Author

Related babashka/babashka#1839

@borkdude
Copy link

I had done some work on this too here: borkdude@40305ce

Feel free to borrow from that if you want too.

Should be working, with the notable exceptions of `:clj-reload/keep` metadata in deftype/defrecord/defprotocol.

I traced it down to how `clj-reload.keep/patch-file` will create an import like `(clojure.core/import clj_reload.keep_deftype.TypeKeep)` in `keep-type-test`, but in Babashka that's not available anymore after `clj-reload.core/ns-unload` calls `(remove-ns ns)`. So in bb we get `java.lang.Exception: Unable to resolve classname: clj_reload.keep_deftype.TypeKeep`.

I'm guessing Clojure's `remove-ns` doesn't remove the Java types but in Babashka it does.
Copy link
Owner

@tonsky tonsky left a comment

Choose a reason for hiding this comment

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

All good with 1 note


(defn meta= [a b]
(= (dissoc (meta a) :ns) (dissoc (meta b) :ns)))
(= (dissoc (meta a) :ns :file) (dissoc (meta b) :ns :file)))
Copy link
Owner

Choose a reason for hiding this comment

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

Actually I would like to keep the check here. Happy to skip it for bb? only and leave it for Clojure

@tonsky tonsky merged commit 7611fce into tonsky:main Sep 15, 2025
@tonsky
Copy link
Owner

tonsky commented Sep 15, 2025

Thanks! Merged

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.

3 participants