Skip to content

Conversation

@rafonsecad
Copy link
Contributor

No description provided.

@rafonsecad rafonsecad mentioned this pull request Oct 16, 2025
(is (thrown? Exception (pop \space)))
(is (thrown? Exception (pop [])))
(is (thrown? Exception (pop '())))
(is (thrown? Exception (pop {})))]))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just collapse the :clj and :cljr into :default since they're identical and likely to be the same for future Clojure platforms that get added (e.g. Clojerl). cljs is often the odd platform out that needs a different implementation while all other platforms have the same

@E-A-Griffin
Copy link
Collaborator

Can you add an assertion for pop's behavior with clojure.lang.PersistentQueue? Most Clojure dialects support this, cljs is the only one I know of that doesn't

@E-A-Griffin
Copy link
Collaborator

clojure.lang.MapEntry also implements pop but that feels more obscure so I'll defer to @jeaye and you on whether we need a test for that; I don't have a strong opinion.

@jeaye
Copy link
Member

jeaye commented Oct 16, 2025

Can you add an assertion for pop's behavior with clojure.lang.PersistentQueue? Most Clojure dialects support this, cljs is the only one I know of that doesn't

Does Clojure CLR have this? I think we should focus more on clojure.core and not the internals of Clojure JVM.

clojure.lang.MapEntry also implements pop but that feels more obscure so I'll defer to @jeaye and you on whether we need a test for that; I don't have a strong opinion.

No, we should focus on pop, not all of the details of Clojure JVM's object model. Some Clojure dialects don't have a dedicated map entry type, since a vector of two suffices.

@E-A-Griffin
Copy link
Collaborator

Can you add an assertion for pop's behavior with clojure.lang.PersistentQueue? Most Clojure dialects support this, cljs is the only one I know of that doesn't

Does Clojure CLR have this? I think we should focus more on clojure.core and not the internals of Clojure JVM.

clojure.lang.MapEntry also implements pop but that feels more obscure so I'll defer to @jeaye and you on whether we need a test for that; I don't have a strong opinion.

No, we should focus on pop, not all of the details of Clojure JVM's object model. Some Clojure dialects don't have a dedicated map entry type, since a vector of two suffices.

Clojure CLR does have this, I would assume any public data structure in clojure.lang.* would be intended behavior rather than JVM implementation details

@E-A-Griffin
Copy link
Collaborator

Looking at other Clojure platforms it looks like
Has clojure.lang.PersistentQueue

  • Clojure
  • bb
  • ClojureCLR

Doesn't have clojure.lang.PersistentQueue

  • ClojureScript
  • nbb
  • Clojerl
  • ClojureDart (didn't test this one but saw some tests commented out from skimming)

@jeaye
Copy link
Member

jeaye commented Oct 16, 2025

Looking at other Clojure platforms it looks like Has clojure.lang.PersistentQueue

* Clojure

* bb

* ClojureCLR

Doesn't have clojure.lang.PersistentQueue

* ClojureScript

* nbb

* Clojerl

* ClojureDart (didn't test this one but saw some tests commented out from skimming)

jank will be in the latter list as well. To me, clojure.lang.* is an implementation detail of Clojure JVM. bb gets it for free, so really only Clojure CLR did the work to implement this.

@rafonsecad
Copy link
Contributor Author

I replaced :clj and :cljr with :default as @E-A-Griffin suggested. Let me know if we need to change something else

@E-A-Griffin
Copy link
Collaborator

@jeaye makes sense to me, and thanks @rafonsecad !

@jeaye jeaye merged commit 77b4b83 into jank-lang:main Oct 17, 2025
2 checks passed
@jeaye
Copy link
Member

jeaye commented Oct 17, 2025

Thank you!

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