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

Support querying subprocess for completion. #119

Closed
wants to merge 9 commits into from
Closed

Support querying subprocess for completion. #119

wants to merge 9 commits into from

Conversation

technomancy
Copy link

Fixes #87.

I ended up disabling the feature that pulls completion targets from locals by default since it can cause unexpected side-effects, but it's documented in the readme how to turn it on.

@technomancy
Copy link
Author

Another note is that this almost works for completion in the lua subprocess buffer, except evaluating the string to determine the completions causes the prompt to be re-printed, which throws off the function which inserts the completions. If there were a way to suppress the re-printing of the prompt, it would solve that problem.

@immerrr
Copy link
Owner

immerrr commented Jun 7, 2016

Hi and thank you for a PR for a long waited feature.

lua-mode.el Outdated
@@ -769,6 +769,9 @@ Groups 6-9 can be used in any of argument regexps."
(setq mode-popup-menu
(cons (concat mode-name " Mode Commands") lua-emacs-menu)))

(make-variable-buffer-local 'completion-at-point-functions)
Copy link
Owner

Choose a reason for hiding this comment

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

For making other people's variables local just in this mode it's better to use make-local-variable.

@immerrr
Copy link
Owner

immerrr commented Jun 7, 2016

I have been meaning to integrate Jupyter for REPL purposes for ages, but never had time for it. Your solution has the undeniable benefit of not having any extra dependencies and may work for now. One thing though is that I'm not very comfortable accepting it without any tests. Could you add some?

lua-mode.el Outdated

Queries current lua subprocess for possible completions."
(let* ((start-of-expr (save-excursion
(search-backward-regexp "[^\.a-zA-Z0-9_]") (point)))
Copy link
Owner

Choose a reason for hiding this comment

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

Does it handle : as binding attribute reference (or whatever is it actually called in Lua)? Can it?

Same questions for whitespace between components? E.g.

my_super_long_variable_name.
    super_long_attribute_name

@technomancy
Copy link
Author

I've made all the changes you suggested, though the last one was pretty tricky!

I took a look at adding tests, and I am not really even sure how to run the tests that are included so far. I avoid elpa/melpa for security reasons since Emacs cannot safely make TLS connections (https://glyph.twistedmatrix.com/2015/11/editor-malware.html); I tried to get the tests to run by downloading buttercup manually but didn't have any success with that. Do you have any recommendations for how that would work?

@immerrr
Copy link
Owner

immerrr commented Jun 7, 2016

I've made all the changes you suggested, though the last one was pretty tricky!

Great, thank you!

Emacs cannot safely make TLS connections

Woah, thank you for the heads up, never came up to me that Emacs is that careless.

Do you have any recommendations for how that would work?

There's package-install-file function that could be run on a local tar archive, will that work for you?

@technomancy
Copy link
Author

technomancy commented Jun 8, 2016 via email

@immerrr
Copy link
Owner

immerrr commented Jun 8, 2016

Something like this should work:

emacs -batch -l lua-mode.el -l buttercup -f buttercup-run-discover

@technomancy
Copy link
Author

technomancy commented Jun 8, 2016 via email

@immerrr
Copy link
Owner

immerrr commented Jun 9, 2016

I'll do my best to find time to give it a spin on the weekend and merge, thank you for your effort.

@immerrr
Copy link
Owner

immerrr commented Feb 7, 2018

Hi!

Apologies for my replies that take eternity, I rebased it, tried it around a bit, done a couple of changes in https://github.com/immerrr/lua-mode/tree/pr-119, feel free to reset your head on top of it or use it as a reference. Aside from that, it looks great, thank you!

Have you signed an FSF copyright assignment by chance?

@immerrr
Copy link
Owner

immerrr commented Feb 7, 2018

Whoops, I seem to have broken something with my changes...

@technomancy
Copy link
Author

I am seeing breakage, but it's not related to your change. It's when you try to do completion at the end of a function and you get [string "REPL"]:1: unfinished string near ''end'. Should be an easy fix; it's just miscalculating the boundaries of the input.

I have signed copyright paperwork; is lua-mode going to be integrated into Emacs? That's great news.

I realized that this code could be improved a fair bit by including top-level locals in the completion targets; would you like me to add that on your branch along with the bug fix?

@immerrr
Copy link
Owner

immerrr commented Feb 9, 2018

I am seeing breakage, but it's not related to your change.

I was trying to replace replace-regexp (which generates a warning for being intended to be used interactively) with replace-regexp-in-string, but tests started failing afterwards. I'll have a look at it on Saturday if you don't beat me to it.

I have signed copyright paperwork; is lua-mode going to be integrated into Emacs? That's great news.

That was the intention a while ago. There's still a big blocker in the shape of the indentation engine, that comes from the ages when no one cared about copyright assignments, but I'm trying to at least avoid creating more.

Although the way Emacs development goes right now, in all likelihood the only win would be putting lua-mode to elpa instead of (or in addition to) melpa.

I realized that this code could be improved a fair bit by including top-level locals in the completion targets; would you like me to add that on your branch along with the bug fix?

That would be nice to have. And you don't have to rush it to get to this PR.

@technomancy
Copy link
Author

Added a bugfix as well as the top-level locals onto your pr-119 branch.

@immerrr
Copy link
Owner

immerrr commented Apr 2, 2018

CI still seems to fail, do you see how to fix it?

@jdtsmith
Copy link

jdtsmith commented May 7, 2019

Seems like this was so close to completion (hah). Any chance to integrate this PR and roll out a new release? Thanks for lua-mode.

@immerrr
Copy link
Owner

immerrr commented May 7, 2019

@jdtsmith I am all for it, but unfortunately don't have time to debug this.

@jdtsmith jdtsmith mentioned this pull request May 8, 2019
@jdtsmith
Copy link

jdtsmith commented May 8, 2019

I found a few issues with @technomancy's PR and generated a new corrected PR #147.

@technomancy
Copy link
Author

Looks like this one is no longer needed; thanks for taking it over!

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.

company backend
3 participants