This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2015-08-07
Channels
- # admin-announcements (4)
- # beginners (11)
- # boot (94)
- # cider (12)
- # cljs-dev (87)
- # clojure (64)
- # clojure-austin (1)
- # clojure-australia (3)
- # clojure-canada (1)
- # clojure-dev (7)
- # clojure-russia (28)
- # clojurescript (292)
- # datomic (2)
- # editors (1)
- # events (4)
- # hoplon (35)
- # instaparse (56)
- # ldnclj (10)
- # off-topic (141)
- # re-frame (2)
- # reagent (2)
@dolen created my first two clojurescript issues, unsure of whether the format is acceptable
also happy to tackle either if they’re beginner friendly (don’t want to make the mistake of jumping into a deeper hole than I expected though)
@crisptrutski: very wary of 1399, that ticket will need more justification. 1400 needs a ClojureScript only reproducer.
ok thanks, i’ll update the tickets
for 1399 - think rejecting #”…”
contents that are not valid “…”
expressions is a fine outcome - just current behaviour was very confusing
@crisptrutski: 1399, but it’s open set, the ticket needs to describe how this can be accomplished reasonably.
@dnolen: updated 1400 with a minimal repo, and here’s a quick-link to the meat https://github.com/crisptrutski/do-doseq-bootstrapped/blob/master/src/do_doseq_bootstrapped/core.cljs
@crisptrutski: please add reproducer inline in the ticket thanks
i’ve linked to the repo in the ticket - is there a better way?
@crisptrutski: just put it inline in the ticket, it’s going to be small
whoops, too late updated again
will use comments in future to decrease noise, sure
@crisptrutski: in this case fine
@crisptrutski: 1400 vetted & assigned to you
awesome
any tips for digging in? very green to the compiler
@crisptrutski: ticket updated with guidance about where to add the test case
@crisptrutski: look at doseq
in src/main/clojure/cljs/core.cljc
ok great - yeah looked at the macroexpansion already
because of redefinition we have to be a bit careful when we need the “host” macro functionality
gotcha
but this gets a little more mind-bending because we do the same thing for bootstrapped ClojureScript
where core/str
now refers to ClojureScript macros/fns because we redefining them again so that they can be compiled to JavaScript
the next thing that your test cases demonstrates, it appears to depend on the :context
thanks for taking the time to explain this - looks like i’m about to learn a lot
have an idea for 1399, will try put a patch together for that also
I’ll be working on various ClojureScript things today so feel free to ping me w/ more questions
great, thanks!
@crisptrutski: better to explain what you’re going to do before starting on patch so you don’t get disappointed if it gets rejected after you work on it
i’ve only got an hour for this, so will probably get over the hump during the weekend
haha ok
changing the behaviour of the regex reader macro to validate the string the same way the string form does
@crisptrutski: the reader isn’t a part of ClojureScript though, that’s tools.reader
ok, i was looking at cljs.reader
the macros function - is that code deprecated?
ok, guess this is losing legs then
#”\/“ is perfectly legal in regular clojure
so suggestion then would just be to treat escaped slashes as regular slashes in the regex generation, like clojure does
@crisptrutski: ok seems reasonable, but definitely a tools.reader thing
thanks for heading off my wrong turn - interested to poke at the project too so it’s a plus
@crisptrutski: if that’s the approach you want to take I would close 1399 with comment then open tools.reader issue and cross reference.
just to node, the behaviour of (cljs.reader/read-string "#\"\/\””)
is exactly to throw on the string form
just to double check my understanding before closing 1399
clojure itself is not using tools.reader? trying to understand how only cljs is getting poisoned
guessing “\/“
is read as “\\/“
by tools.reader, but doubt clojure would demunge that
as far as I remember, #”\/“ => “\\/“
which is correct for clojure
well (re-find #"\/" "/“) => “/"
I’d guess that works due to the behaviour of java.util.regex.Pattern
Just a guess though
@dnodel - just tried (clojure.tools.reader/read-string "#\"\/\””)
, and that does throw - so the issue must be on the cljs side still
@andrewmcveigh: (re-find #”\\/" "/“) => nil
@crisptrutski: but your first example is not the same
You need to escape that backslash… (clojure.tools.reader/read-string "#\"\\/\”")
^^ works fine
what a silly mistake - thanks for spotting it
The second example is looking for a literal backslash, which is obviously not there
sure - understand the behaviour of the valid regexes, just trying to see where js and jvm routes diverge for the badly formed string
so seems the regex form isn’t read by tools.reader for clj
Clojure proper has it’s own reader written in java
and still confused at how “`#”\\/“`" via #“\/“
results in an unescaped /
in the js regex, but #”/“ emits an escaped one
sure people are tired of this noise for such a trivial edge case though - i’m shutting up
@crisptrutski: look at the emission for Pattern
and RegExp
in cljs/compiler.cljc
we do some escaping there
Yep, was just looking at that.
ah, ok so this double escaping of slashes caught me out
#”\/“
, (str (r/read-string "#\"\\/\"”)) => "\\/“
#”\\/“
, (str (tools.reader/read-string "#\"\\\\/\"”)) => "\\\\/“
so that’s a smoking gun for tools.reader
if “\\\\” is one literal “\”, then what is “\\/“ - junk
and the clojurescript emitter was not written defensively around invalid regex asts