Port to cljc #3

PEZ · 2019-10-06T17:35:55Z

Hello,

This is work In progress. Wanted to get your feedback on wether you think it is the right way to go or not. For me it is important to be able to use a library like this from ClojureScript, because that's what Calva needs.

Right now do not expect anything to really work. Running the node tests fails like so:

$ node target/out/tests.js 
/Users/pez/Projects/parcera/target/out/cljs/core.js:32980
return (new RegExp(pattern,(function (){var or__7908__auto__ = flags;
        ^

SyntaxError: Invalid regular expression: /\P{M}\p{M}*+/: Nothing to repeat
    at new RegExp (<anonymous>)
    at cljs$core$re_pattern (/Users/pez/Projects/parcera/target/out/cljs/core.js:32980:9)
    at instaparse$cfg$process_regexp (/Users/pez/Projects/parcera/target/out/instaparse/cfg.js:143:41)
    at instaparse$cfg$build_rule (/Users/pez/Projects/parcera/target/out/instaparse/cfg.js:196:85)
    at /Users/pez/Projects/parcera/target/out/cljs/core.js:17671:89
    at /Users/pez/Projects/parcera/target/out/cljs/core.js:17672:3
    at Object.sval (/Users/pez/Projects/parcera/target/out/cljs/core.js:11499:109)
    at Object.cljs$core$ISeqable$_seq$arity$1 (/Users/pez/Projects/parcera/target/out/cljs/core.js:11650:10)
    at Object.cljs$core$_seq [as _seq] (/Users/pez/Projects/parcera/target/out/cljs/core.js:2424:10)
    at Object.cljs$core$seq [as seq] (/Users/pez/Projects/parcera/target/out/cljs/core.js:4423:18)

carocad · 2019-10-06T17:59:28Z

@PEZ thanks a lot for the effort. I am not very proficient with the Cljs side but I would guess that the StringBuffer implementation that @borkdude proposed would be enough for this use case. See #2 . From what I see both "classes" provide an append and a toString method which I guess should be enough to get this done.

SyntaxError: Invalid regular expression: /\P{M}\p{M}*+/: Nothing to repeat

Regarding the regular expression. That is a special (multicharacter) unicode regex. It might be that Javascript doesnt support that exact syntax so you would need to check that. I put the link from the regex next to the grammar definition for specific rule: https://github.com/carocad/parcera/blob/master/src/parcera/core.clj#L138

Hope it helps

PEZ · 2019-10-06T19:18:54Z

Yeah, maybe we can use that StringBuffer instead. Even if the tiny StringBuilder I used makes for fewer changes in the rest of the code. I.e.something like:

(let [string-builder (new StringBuilder)]
  ...

Stays the same, no need for the conditional reader there. It's a matter of taste, I think. (Though, I say that without having had a look at goop's implementation.)

Will have a look at the unicode regex thing. I've fought that fight before. Seems to vary a bit between node versions what is supported and not...

PEZ · 2019-10-06T20:27:38Z

It seems that the current regex doesn't work for anything unicode as it is (regardless of CLJ or CLJS). Just sticking a Swedish ö in the backticks test:

  (testing "backtick"
    (as-> "`hellö/world" input (is (= input (parcera/code (parcera/clojure input)))))
    (as-> "`hello" input (is (= input (parcera/code (parcera/clojure input)))))
    (as-> "`/" input (is (= input (parcera/code (parcera/clojure input))))))

I get this:

ERROR in (macros) (core.cljc:159)
backtick
expected: (= input (parcera/code (parcera/clojure input)))
  actual: java.lang.IllegalArgumentException: No matching clause: [:index 5]

carocad · 2019-10-06T21:12:56Z

It seems like you just caught a bug 😅. I was using autogenerated symbols to test the symbol-regex but it seems that it doesnt create symbols with unicode characters in it.

However, the regex that I mentioned was only for the character literals not for symbols which is why you just saw this behavior.

Also add test for character literals

PEZ · 2019-10-06T22:10:47Z

I see. I think I just assumed that the simple characters built up the identifiers. Should have read more carefully.

Anyway. I've used http://kourge.net/projects/regexp-unicode-block to cook the character classes for the regex, since JavaScript just does not have the unicode support in its regex engine. Added a test for character literals, including unicode ones, which passes in both Clojure and ClojureScript. What'ya say?

PEZ · 2019-10-06T22:12:46Z

Right now the test suite as such bails on the (slurp), which does not exist in CLJS. Otherwise everything that passes in CLJ passes in CLJS too.

carocad · 2019-10-06T22:34:08Z

Right now the test suite as such bails on the (slurp), which does not exist in CLJS.

I guess you can make a macro that puts the result of slurp into a var that can later on be used for the tests. That way the slurp process is at compile time (clojure) not runtime cljs.

PEZ · 2019-10-06T23:41:02Z

Right, did that now.

lein test
Warning: implicit hook found: leiningen.cljsbuild/activate 
Hooks are deprecated and will be removed in a future version.
Compiling ClojureScript...
Compiling ["target/out/tests.js"] from ["src" "test"]...
Successfully compiled ["target/out/tests.js"] in 6.827 seconds.
Compiling ClojureScript...

lein test parcera.test.core
"Elapsed time: 12232.174756 msecs"
"Elapsed time: 6812.244218 msecs"

Ran 5 tests containing 52 assertions.
0 failures, 0 errors.
Running ClojureScript test: dev

Testing parcera.test.core
"Elapsed time: 26574.280114 msecs"
"Elapsed time: 14582.774624 msecs"

Ran 5 tests containing 52 assertions.
0 failures, 0 errors.

The ClojureScript tests run significantly slower. Maybe that is what It is, but also maybe my naive StringBuilder isn't fast enough. (Can't see why it wouldn't be though.)

carocad · 2019-10-07T08:32:35Z

The ClojureScript tests run significantly slower

I guess one part of it is due to the use of instaparse/parser instead of instaparse/defparaser as mentioned in the docs.

I think another thing that we can check are instaparse tests itself. I would assume that it also has some cljs tests with the same content as those in clj so if those are also significantly slower then there is little we can do :/

Fixes carocad#2

PEZ · 2019-10-07T10:34:07Z

After checking up the StringBuffer implementation, and confirmed that it has the same performance as my own tiny shim, I decided that it is worth using the goog implementation instead.

I'll confirm that I can use this library from Calva (shadow-cljs) and if that works, I'd say this is good to go.

it will be easier for more people to obsess on the performance side of things if it gets merged. 😄

project.clj

src/parcera/core.cljc

project.clj

See: https://clojure.atlassian.net/browse/CLJS-1792

Document the character literal unicode regex some. Also moved up criterium in global dependencies. I don't like it, but don't know how to otherwise satsify the production build

project.clj

carocad · 2019-10-09T19:33:33Z

@PEZ sorry for creating the merge conflicts between this PR and the current master. As you know parcera is still in its infancy and I want to get it as correct as possible in the meantime.

By the way, #12 should help you run the tests directly on Travis CI. Hope it helps

PEZ · 2019-10-09T21:41:01Z

I am happy to keep merging your changes from upstream! No worries.

And the JS tests are ... PASSING! Yay!

carocad

Thanks a lot for this improvement :)

Start porting to cljc

0b2857d

carocad mentioned this pull request Oct 6, 2019

symbol name pattern doesnt match unicode characters #5

Closed

Change to a JS compatible unicode regex

56890b8

Also add test for character literals

Add slurp macro for :cljs

5c2b4ca

carocad and others added 2 commits October 7, 2019 11:28

Merge branch 'master' into wip/cljc

33a135a

Use StringBuffer instead of own StringBuilder

9558152

Fixes carocad#2

carocad reviewed Oct 7, 2019

View reviewed changes

project.clj Outdated Show resolved Hide resolved

carocad reviewed Oct 7, 2019

View reviewed changes

src/parcera/core.cljc Outdated Show resolved Hide resolved

carocad reviewed Oct 7, 2019

View reviewed changes

project.clj Outdated Show resolved Hide resolved

PEZ added 2 commits October 7, 2019 13:06

Add test.check dependency

70b54ce

See: https://clojure.atlassian.net/browse/CLJS-1792

Move slurp.cljc out of test dir, b/c test-runner

6cbc0ff

Document the character literal unicode regex some. Also moved up criterium in global dependencies. I don't like it, but don't know how to otherwise satsify the production build

PEZ changed the title ~~[WIP] Start porting to cljc~~ Port to cljc Oct 7, 2019

carocad reviewed Oct 7, 2019

View reviewed changes

project.clj Outdated Show resolved Hide resolved

PEZ and others added 7 commits October 8, 2019 16:14

Moved test dependencies back to dev profile

0f29bac

Rename test-command to test and remove hook

defae59

Remove global :cljsbuild entry

189e5be

allow unicode in name definitions

5d5c2d2

version bumped

91f4d13

Add some tests for names

6871e23

try to get java and nodejs in the same build setup

b6c91b4

carocad mentioned this pull request Oct 9, 2019

change CI job to have both Java and Node.js installed #12

Merged

Merge branch 'master' into prepare-js

f13a664

PEZ added 4 commits October 9, 2019 23:16

Merge remote-tracking branch 'upstream/master' into wip/cljc

ee32db1

Merge remote-tracking branch 'upstream/prepare-js' into wip/cljc

98cc823

Change parse self paths to cljc

fd31486

Add command for running actual js tests

3f3ae57

carocad approved these changes Oct 9, 2019

View reviewed changes

carocad merged commit 91f911c into carocad:master Oct 9, 2019

PEZ deleted the wip/cljc branch October 9, 2019 22:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port to cljc #3

Port to cljc #3

PEZ commented Oct 6, 2019

carocad commented Oct 6, 2019 •

edited

PEZ commented Oct 6, 2019

PEZ commented Oct 6, 2019

carocad commented Oct 6, 2019

PEZ commented Oct 6, 2019

PEZ commented Oct 6, 2019

carocad commented Oct 6, 2019

PEZ commented Oct 6, 2019

carocad commented Oct 7, 2019

PEZ commented Oct 7, 2019

carocad commented Oct 9, 2019

PEZ commented Oct 9, 2019

carocad left a comment

Port to cljc #3

Port to cljc #3

Conversation

PEZ commented Oct 6, 2019

carocad commented Oct 6, 2019 • edited

PEZ commented Oct 6, 2019

PEZ commented Oct 6, 2019

carocad commented Oct 6, 2019

PEZ commented Oct 6, 2019

PEZ commented Oct 6, 2019

carocad commented Oct 6, 2019

PEZ commented Oct 6, 2019

carocad commented Oct 7, 2019

PEZ commented Oct 7, 2019

carocad commented Oct 9, 2019

PEZ commented Oct 9, 2019

carocad left a comment

Choose a reason for hiding this comment

carocad commented Oct 6, 2019 •

edited