lynaghk / cljx

Write a portable codebase targeting Clojure/ClojureScript
Other
398 stars 34 forks source link

Bugfixes (#61 and #46) and minor performance improvements. #62

Closed niwinz closed 9 years ago

niwinz commented 9 years ago

Hi.

This pr includes:

  1. Fix for issue #61. That issue causes a lot of unexpected exceptions using cljx together with speclj.
  2. Improvement on partial building: relativize path process changed from use of URI to Path, that handles much better paths that URI and not prepends unexpecred file; and now a filtes are filtered by build source-path.
  3. Fix for issue #46: polling reate drecreased to more apropiate one (100ms)
  4. Minor performance improvement with little cost of additional memmory adding memoized version of transform. (I have tested in my project and memory fooprint is negligible.)
cemerick commented 9 years ago

Thank you very much for the patch. A couple of things:

  1. Can you break this up into one PR for each problem/change? As it stands, it's not easy to tell what each part of the diff is addressing.
  2. Path, etc. requires JDK7+, but ClojureScript only requires JDK6+ IIRC. Could you explain what the issue is that your (2) is addressing?
niwinz commented 9 years ago

Hi Chas!

Ok, I've removed not related commits to #60 issue and I will send them as separate PR.

The previos implementation has two drawbacks:

Appending file: is a proper behavior for URI but not for filesystem paths. For it, nio Path fits much better, because it handles the fs paths much correctly that URI.

In my opinion, we should keep Path, but if your insist on have support for JDK6, I'll try to reimplement the second commit using URI with explicit checking the "file:" prefix, for removing it.

niwinz commented 9 years ago

In any case, each commit has a little explication about it changes (on the body of the commit).

cemerick commented 9 years ago

Yes, let's please keep JDK6 compat. I don't use it, but others might. Tracking the compatibility targets of Clojure and ClojureScript is the right thing to do.

Thanks for the tidying! Looking forward to merging.

imikushin commented 9 years ago

+1 This fix is critical for lein cljx auto to be useful (e.g. with lein cljsbuild auto). I've merged this into my fork. Does the job. (BTW, I don't think JDK6 is still relevant)

niwinz commented 9 years ago

Replaced by new pr, already linked ;)