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

change ns g.t.c.mock to g.t.c.mock-protocol #10

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

HendrikLevering
Copy link

This fixes a bug, which occurs in CLJS, where mock fn overrides the g.t.c.mock namespace completely.
See:
https://clojurians.slack.com/archives/C03S1L9DN/p1732105416688439?thread_ts=1732100469.195189&cid=C03S1L9DN

Copy link

@nha nha left a comment

Choose a reason for hiding this comment

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

Namespace griffin.test.contract.mock clashes with var griffin.test.contract/mock

Indeed. This is described in the cljs docs

Namespaces in ClojureScript are compiled to Google Closure namespaces which are represented as nested JavaScript objects. Importantly this means that namespaces and vars have the potential to clash - however the compiler can detect these problematic cases and will emit a warning when this occurs.

This PR makes sure the above do not clash, by renaming the protocol. This is a breaking change, as the contract protocol namespace changes.

(swap! s f)
this))))

(defn var->sym [v]
(symbol (str (-> v meta :ns ns-name)) (str (-> v meta :name))))
Copy link

Choose a reason for hiding this comment

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

[Praise] extracting this in a function makes sense.
[Question] why is the body written differently?

Copy link

Choose a reason for hiding this comment

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

The reason I am asking is that I haven't found an authoritative source that would guarantee the metadata or the static fields ns and name to be present.

The options I can see:

  • (.ns v) and (.sym v) have the advantage that if these were taken away in a subsequent Clojure release this code would break
  • (-> v meta :ns ns-name) avoids relying on java interop
  • clojure.lang.Var/.toSymbol also does the same

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing it's because (.ns v) isn't portable to ClojureScript, but (-> v meta :ns) is.

@@ -186,5 +186,5 @@
(deftest broken-model-test
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#":args must return a generator for"
#":args must return a generator for*"
Copy link

Choose a reason for hiding this comment

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

[Question] Was this * intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

It'll match r*

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.

4 participants