Issues 296 and 300 - Add a macro for common tests between Clojure and ClojureScript #301
Conversation
…onment when compiling ClojureScript
|
For some reason Travis CI doesn't seem to be picking up /src/test/common although my local builds do. This looks like it is an issue with master too though. Clarification: It isn't running them in Clojure, but it is running them in ClojureScript. |
| ;; to resolve the symbol in the ClojureScript compiler's | ||
| ;; Clojure environment. See issue 300. | ||
| (not (com/compiling-cljs?)) | ||
| (resolve (first condition)))] |
There was a problem hiding this comment.
It does seem odd to me that the clj impl would embed a class object in the data structure at this point. A fully-qualified symbol seems more appropriate and a "data representation". The fact that clj lets you embed class objects in eval code etc just seems to be non-documented behaviors.
This is a broader topic though, so not really suitable to be fixed in this issue.
There was a problem hiding this comment.
Yeah, we can drop this in another change. I originally had wanted to validate the presence of a type early in the process to fail fast, but it doesn't make sense to include in our structure.
| cat))) | ||
|
|
||
| test-form `(~(if (com/compiling-cljs?) | ||
| 'cljs.test/deftest |
There was a problem hiding this comment.
This is sort of a macro-anti-pattern to return code that has implicit dependencies on namespaces being pre-required by the caller.
If this macro was used by a caller that did not have a require on cljs.test (or clj) and it wasn't already loaded elsewhere, then it would fail.
There was a problem hiding this comment.
I discussed this some in the docstring here. I agree that it is undesirable; unfortunately, my understanding is that ClojureScript doesn't have good support for macros causing additional namespaces to be required since all requires have to be at the beginning of the file. I'd be happy to find out I'm wrong about that or that there is a way around this problem though, and I'm certainly not as well-versed in ClojureScript's compilation model as in Clojure's. Practically speaking, for this particular use-case it probably isn't a big deal since I'd expect the namespaces consuming this to have those requires anyway and this is currently targeted toward consumption within Clara, not as an external API.
There was a problem hiding this comment.
Oh, wow, you explained this already thoroughly in the doc string. I somehow overlooked it.
That makes sense what you are saying.
It'd be pretty tricky to pull of the require on the cljs side I'd think so seems fine for now.
| @@ -0,0 +1,56 @@ | |||
| #?(:clj | |||
There was a problem hiding this comment.
It is somewhat non-idiomatic to use a read conditional at this scope.
It is more common to see them used just in the places where the forms differ. Here there is duplication and even a chance the ns name could be changed, etc.
It's not a huge issue though.
There was a problem hiding this comment.
Yeah, doing it all in one body with reader conditionals is also an option. In this case I just thought that so much differed for such a small ns header that it would be more readable to just have entirely separate forms. Looking back at it I'm thinking perhaps make the namespace name common and just have the rest be separate.
There was a problem hiding this comment.
Having everything other than the name be separate is complicated by having multiple top-level forms beneath the declaration for clj and cljs. I think I'll just keep this as is; by my count only the clara.rules require is identical between clj and cljs so splicing in lots of reader conditions would hurt readability more than it would help it in this instance IMO.
|
Looks good to me! |
No description provided.