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

Cache namespaceinfo in repl buffers #1301

Merged
merged 4 commits into from
Sep 10, 2015
Merged

Cache namespaceinfo in repl buffers #1301

merged 4 commits into from
Sep 10, 2015

Conversation

Malabarba
Copy link
Member

Must go with clojure-emacs/cider-nrepl#249

  • cider-resolve.el has a set of functions for resolving clojure symbols based on what we know in Emacs (without having to query nREPL). These can be used for performance-sensitive contexts, such as indentation and font-locking.
  • This PR also displays how to apply that to font-locking. It implements dynamic font-locking of macros, functions, and vars as they are loaded. Here's an example:
    font-lock
    I think the macro font-locking is pretty nice and should be default. Functions and variables are debatable. Some people might find them excessive so they might be better turned off by default. But I'd like to hear opinions.

@arrdem
Copy link
Contributor

arrdem commented Sep 3, 2015

👍 awesome! General macro font locking is something I've wanted for a while.

Now if you could get local font locking to work 😉

@bbatsov
Copy link
Member

bbatsov commented Sep 3, 2015

Now if you could get local font locking to work

I don't quite get this. Most IDEs/editors would not font-lock locals anywhere. And there's also the problem of figuring out what's a local - compliment has some limited local inference, but something general will be pretty hard to do.

@Malabarba
Copy link
Member Author

I can think of pretty foolproof ways for the middleware to detect which symbols are locals in each scope.
However, there's no easy way for the client and server to communicate such complicated details, and on top of that the emacs font locking system isn't designed to handle local symbols.

So, not likely to happen. :-)

@Malabarba
Copy link
Member Author

Ok, I have added a nice defcustom to the PR. Now the default is to font-lock all macros (with font-lock-builtin-face) and to font-lock anything from clojure.core (with the proper face depending on whether it's a macro, function, or var).

@bbatsov
Copy link
Member

bbatsov commented Sep 4, 2015

Finally had a time to go over the PR and most things look. My only real concern is the code copy-pasted from clojure-mode and keeping it in sync going forward.

@Malabarba
Copy link
Member Author

Yeah, I wouldn't want it get outdated, but I'm not sure what could be done about it. =/

@bbatsov
Copy link
Member

bbatsov commented Sep 4, 2015

Well, I guess we can live this for now. Apart from my inline comments here's a few more:

  • the dynamic font-locking should definitely be in the README and the CHANGELOG
  • all font-locking commits should be squashed together
  • same goes for commits related to resolve

@bbatsov
Copy link
Member

bbatsov commented Sep 6, 2015

You'll also have to rebase this.

@Malabarba
Copy link
Member Author

Actually, I'm rewriting this now.
The performance was not satisfactory, so I'll have to switch to static font-lock rules, instead of deferring to a function call.

@Malabarba Malabarba force-pushed the track-ns branch 2 times, most recently from f3451e6 to f0a0775 Compare September 8, 2015 09:58
@Malabarba
Copy link
Member Author

OK, now I'm much more satisfied with the performance. I think it's a little simpler as well.

@bbatsov
Copy link
Member

bbatsov commented Sep 8, 2015

Yeah, looks simpler to me as well. I think you only need to update the save-match-data's location and we're good to go.

@Malabarba
Copy link
Member Author

Removed the save-match-data completely, it's no longer necessary.
Also added cljs support and fixed a couple of kinks. I think it's ready now.

Ditch instrumented defs overlay for the new font-locking.
All macros are now font-locked.
This is configurable via the cider-font-lock-dynamically variable.
bbatsov added a commit that referenced this pull request Sep 10, 2015
Cache namespaceinfo in repl buffers
@bbatsov bbatsov merged commit ac255b9 into master Sep 10, 2015
@bbatsov bbatsov deleted the track-ns branch September 10, 2015 05:42
@bbatsov
Copy link
Member

bbatsov commented Sep 10, 2015

OK, let's release this into the wild. :-)

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.

3 participants