Skip to content

Conversation

@OknoLombarda
Copy link
Contributor

Follow up to #630

Forgot to make function definition regexp only look for defn forms. This PR will fix such behaviour:
image


Before submitting a PR mark the checkboxes for the items you've done (if you
think a checkbox does not apply, then leave it unchecked):

  • The commits are consistent with our contribution guidelines.
  • You've added tests (if possible) to cover your change(s). Bugfix, indentation, and font-lock tests are extremely important!
  • You've run M-x checkdoc and fixed any warnings in the code you've written.
  • You've updated the changelog (if adding/changing user-visible functionality).
  • You've updated the readme (if adding/changing user-visible functionality).

Thanks!

@OknoLombarda
Copy link
Contributor Author

OknoLombarda commented Sep 3, 2022

Maybe it's better to also remove optional namespace from regexp? I mean, to allow only clojure.core

(,(concat "(\\(?:" clojure--sym-regexp "/\\)?"

@OknoLombarda OknoLombarda changed the title Fix second element of forms starting with 'def' being highlighted as function names Fix second element of forms starting with 'def' being highlighted as function name Sep 3, 2022
@bbatsov
Copy link
Member

bbatsov commented Sep 3, 2022

Yeah, that'd be better. It used to accept all namespaces just because it could be anything.

@OknoLombarda
Copy link
Contributor Author

Done

(5 5 nil)
(6 9 font-lock-keyword-face)
(11 13 font-lock-function-name-face))
(11 13 nil))
Copy link
Member

Choose a reason for hiding this comment

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

I guess this example should be updated to use clojure-core, otherwise it's kind of pointless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, makes sense. Done

(5 5 nil)
(6 14 font-lock-keyword-face)
(16 18 font-lock-function-name-face))
(16 18 nil))
Copy link
Member

Choose a reason for hiding this comment

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

Seems to me this example needs to be updated as well to use clojure.core, as I doubt they would be a different ns for defrecord. Also we should probably make defrecord and deftype use font-lock-type-face for the name of the type.

Copy link
Contributor Author

@OknoLombarda OknoLombarda Sep 3, 2022

Choose a reason for hiding this comment

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

Replaced namespace with clojure.core.

Also we should probably make defrecord and deftype use font-lock-type-face for the name of the type.

We already do, I didn't change that. I changed the test to check if this face is used

Copy link
Member

Choose a reason for hiding this comment

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

Isn't 16-18 the name of the type foo? It seems it has no font-locking now.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see this wasn't working because of the wrong namespace. My bad.

@bbatsov
Copy link
Member

bbatsov commented Sep 3, 2022

It's good we have so many tests - they helped me spot two weird behaviors currently that it would be nice to address.

@bbatsov bbatsov merged commit 24ee368 into clojure-emacs:master Sep 3, 2022
@bbatsov
Copy link
Member

bbatsov commented Sep 3, 2022

Looks good now. Thanks!

@OknoLombarda OknoLombarda deleted the fix-forms-with-def branch September 3, 2022 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants