Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(clojure) Add support for global definitions name (class: title) #2347

Closed
wants to merge 0 commits into from

Conversation

agrison
Copy link
Contributor

@agrison agrison commented Jan 9, 2020

Adds support for Clojure global definitions name with class name title, including:

  • def
  • defonce
  • defprotocol
  • defstruct
  • defmulti
  • defmethod
  • defn
  • defn-
  • defmacro
  • deftype
  • defrecord

@joshgoebel
Copy link
Member

Wouldn't this patch remove the need for NAME and BODY?

@joshgoebel
Copy link
Member

Please also add a changelog entry.

Website: https://clojure.org
Category: lisp
*/

function(hljs) {
var globals = 'def defonce defprotocol defstruct defmulti defmethod defn defn- defmacro deftype defrecord';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a space here.

If these so different might one of the other keyword classes be more appropriate also?

(<span class="hljs-keyword">defonce</span> ^<span class="hljs-symbol">:private</span> <span class="hljs-title">another-var</span> <span class="hljs-title">#</span><span class="hljs-string">"foo"</span>)

<span class="hljs-comment">; private function</span>
(<span class="hljs-keyword">defn</span><span class="hljs-title">-</span> <span class="hljs-title">add</span> [x y] (<span class="hljs-name"><span class="hljs-builtin-name">+</span></span> x y))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct. - is not the title. You'll need to update lexeme so it can match keywords that include '.

@joshgoebel
Copy link
Member

Ping.

@joshgoebel joshgoebel added needs work language enhancement An enhancement or new feature labels Jan 15, 2020
@agrison
Copy link
Contributor Author

agrison commented Jan 15, 2020

Thanks for the feed-back it's my first time on highlightjs so didn't knew all the specifics I'll update with changes whenever I do have some free time :)

@joshgoebel
Copy link
Member

Ping.

@joshgoebel joshgoebel added this to the 10.0 milestone Jan 31, 2020
@joshgoebel
Copy link
Member

Ping. Still out there?

@joshgoebel
Copy link
Member

Ping.

@agrison
Copy link
Contributor Author

agrison commented Feb 20, 2020

Hello.

Yes still out there, sorry for the delay, I've been busy and couldn't work on this.

Sadly, yesterday I tried but I don't know enough about the inners of the library to make it work like you need to, I tried moving some definitions, modify the lexeme but to no avail, it always break something else, and I don't have much time to investigate for this.

@joshgoebel
Copy link
Member

joshgoebel commented Feb 20, 2020

Ok, Clojure actually is kind of a mess... and you might also be hitting an actual issue/bug with HLJS in that beginKeywords wants keywords to end on a word boundary but the "space" between - and (space) is NOT considered a word boundary evidentally... so it fails to match on: defn-... but that's only part of the problem...

The grammar is making a mess of things by trying to count almost everything as a NAME or a SYMBOL... which I'm pretty sure is wrong, and definitely not consistent with how we do other grammars...

But if you want to help, lets try and start on fixing some smaller things I see first... then maybe after we clean it up a bit we can circle back to this.

First question:

(def +x [(a 1) +2 -3.0 y-5])

What is -5 there? The grammar right now identifies y-5 as a symbol because...

var SYMBOLSTART = 'a-zA-Z_\\-!.?+*=<>&#\'';
var SYMBOL_RE = '[' + SYMBOLSTART + '][' + SYMBOLSTART + '0-9/;:]*';
  
var SYMBOL = {
    begin: SYMBOL_RE,
    relevance: 0
  };
  • Is y-5 truly an identifier/variable all on it's own or is that actually y - 5 (y subtract 5)?
  • Can a variable/identifier TRULY start with - as part of the name? That would seem very confusing.
  • Do you have any idea what the NAME rule is trying to accomplish?

@joshgoebel
Copy link
Member

I might be getting the naming confused... we also have keys, which get the class symbol... but that's not what I'm talking about here. I'm talking about the literal rule that I show the source for above.

@agrison
Copy link
Contributor Author

agrison commented Feb 21, 2020

Well Clojure is not really a mess, the syntax is simple and easy to pick-up, of course it can be a little disturbing the first times.

In your example y-5 is a valid symbol identifier in Clojure, if you wanted y minus 5 it would have been written (- y 5)

Yes a symbol can start with a minus or a plus character.

user=> (def y-5 7)
#'user/y-5
user=> (+ y-5 2)
9
user=> (def -foo "hello")
#'user/-foo
user=> (str -foo " world")
"hello world"
user=> (def -+* (+10 11))
#'user/-+*
user=> (def *-+ 21)
#'user/*-+
user=> (+ -+* *-+)
42

You're probably right, maybe the highlightjs parser is having some limitations that needs to be fixed prior to scan correctly clojure code. But it could also be that I don't know how to use it correctly to find the symbol boundaries effectively.

@joshgoebel
Copy link
Member

Whoops, I accidentally pushed your master back to upstream/master instead of pushing my branch to yours - I forget the syntax cause I rarely do it - and now I can't push it because the PR is closed and the magic permissions go away.

I will re-open a new PR.

@joshgoebel
Copy link
Member

#2419

@joshgoebel
Copy link
Member

@agrison LOL. No I didn't mean Clojure was a mess, I meant the grammar was... though it makes a little more sense with our explanation. Anyways I went back over this and tried to just do the minimal amount to make this work for now.

Basically that meant making sure title only fires a single time and also removing some pieces of your rule that don't seem to have any real effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature language needs work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants