Add CLJS self-require for macros

Description

Repro of current behavior:

Patch TCHECK-154.patch fixes the issue for the generators namespace.

As discussed on Slack:

CLJC namespaces now have a policy of self-requiring macros for CLJS so consumer namespaces don't need to use require-macros/include-macros.

Patch TCHECK-154-2.patch adds self-requires for all relevant namespaces and removes require-macros/include-macros from the testing namespaces to verify that it works as before.

Environment

None

Activity

Show:
Michiel Borkent
February 22, 2019, 6:38 AM

I don't know what the official position of the CLJS maintainers is, but adding this patch will make consuming the let macro less confusing. When you forget to require it as a macro (like I did) you get a version of let that is a normal CLJS function instead of a macro and a surprising error message (like posted in the issue). I now realize that I could have solved this as a user of test.check by using :require-macros in the ns expression of my code. Making it easier to consume the macro is a nice enhancement I would say.

gfredericks
February 25, 2019, 12:48 AM

Is this something that's easy to write an automated test for? It'd also be nice to have the other macros covered. I think that might just be the ones in prop, but I might be forgetting one.

The test would be nice so that an ignorant maintainer doesn't conclude the line is a NOOP and delete it.

Michiel Borkent
February 25, 2019, 9:57 AM

Gary: I've been looking into this. This test would only work if you remove :include-macros like in [clojure.test.check.generators :as gen #?@(:cljs [:include-macros true])] from other test namespaces, else the error won't show up. If you do, the error will show up without the extra self-refer clause. So that would already be the test. I'd be happy to finish this for the rest of the macros and remove :include-macros from the tests, just let me know.

gfredericks
February 25, 2019, 7:24 PM

If the test namespace has to be setup funny, then let's make a new dedicated test namespace for this, and make it clear via names or comments or docstrings what it's for.

gfredericks
March 3, 2019, 3:11 AM

Patch applied, thanks!

Completed

Assignee

gfredericks

Reporter

Michiel Borkent

Labels

None

Approval

None

Patch

Code

Priority

Minor