Skip to content

Fix for #79 #82

New issue

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

By clicking “No 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? No Sign in to your account

Merged
merged 4 commits into from
Jul 6, 2017
Merged

Fix for #79 #82

merged 4 commits into from
Jul 6, 2017

Conversation

arichiardi
Copy link
Contributor

@arichiardi arichiardi commented Jun 6, 2017

Replace this placeholder text with a summary of the changes in your PR.
The more detailed you are, the better.


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • The new code is not generating bytecode or M-x checkdoc warnings
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)

Thanks!

@arichiardi arichiardi changed the title Fix 79 Fix for #79 Jun 6, 2017
@arichiardi arichiardi force-pushed the fix-79 branch 3 times, most recently from 55bac77 to 18a4c0f Compare June 6, 2017 05:42
README.md Outdated
@@ -99,6 +99,26 @@ point. You can, however, change this behaviour by invoking such
commands with a prefix argument. For instance: `C-u C-c C-v` will ask
for the symbol you want to show the docstring for.

#### *WARNING* - Standard REPLs
Copy link
Member

Choose a reason for hiding this comment

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

I think the title of this section is extremely misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Probably this should be in some caveats or faq section of the readme.

Copy link
Member

Choose a reason for hiding this comment

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

Or in configuration.

README.md Outdated

Note that if you decide _NOT_ to use the socket repl, it is highly recommended
you disable output coloring and/or readline facilities: `inf-clojure` does not
filter out ASCII escape characters at the moment and will not behave correctly.
Copy link
Member

Choose a reason for hiding this comment

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

Why filter them out? Why not simply apply them the same way we do in CIDER? It's quite trivial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We apply them visually, but the regex match is failing every time because of those. A solution is for our regexes to skip the ascii escape codes. Do you have a link for cider? If there is an alternative I am all for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue of speaking with the terminal is a big one. Comint hides some of the complexity but cannot eliminate it. Things like embedded readline and colors all change behaviors and I am so far from being an expert that I was even wondering whether console support should be dropped completely at some point (maybe by adopting another Emacs socket-only package). Sooner or later...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...I guess it will happen. Cider does not handle this because of nRepl in between. In any case I can try to escape them, I saw it in some other plugin ...We'll see about the result.

@arichiardi arichiardi mentioned this pull request Jun 6, 2017
2 tasks
@hlolli
Copy link
Contributor

hlolli commented Jun 6, 2017

Checked out this pull requests and this is what I'm getting

WARNING: No such namespace: complete.core, could not locate complete/core.cljs, complete/core.cljc, or Closure namespace "" at line 1 
             ⬆
WARNING: Use of undeclared Var complete.core/completions at line 1 
complete is not defined
	 (evalmachine.<anonymous>:1:1)
	 ContextifyScript.Script.runInThisContext (vm.cljs:44:33)
	 Object.runInThisContext (vm.cljs:116:38)
	 (Object.lumoEval)
	 lumo.repl.caching_node_eval (NO_SOURCE_FILE <embedded>:6133:194)
	 (NO_SOURCE_FILE <embedded>:5372:29)
	 x (NO_SOURCE_FILE <embedded>:5373:13)
	 Object.cljs.js.eval_str_STAR_ (NO_SOURCE_FILE <embedded>:5374:28)
	 Function.cljs.js.eval_str.cljs$core$IFn$_invoke$arity$5 (NO_SOURCE_FILE <embedded>:5377:508)
	 Object.lumo.repl.execute_text (NO_SOURCE_FILE <embedded>:6257:240)

and as I type reduce (which I fixed in my pull request)

Company: backend company-capf error "Wrong type argument: listp, ⬆" with args (candidates reduc)
Company: An error occurred in auto-begin
Company: backend company-capf error "Wrong type argument: listp, ⬆" with args (candidates reduce)

@arichiardi
Copy link
Contributor Author

arichiardi commented Jun 6, 2017 via email

@hlolli
Copy link
Contributor

hlolli commented Jun 6, 2017

On lumo yes (can be confirmed by looking at the stacktrace).

No problem!

@arichiardi
Copy link
Contributor Author

@hlolli how do you launch lumo? Because it does not happen here, I can see the eldoc. Lumo should be launched with -d because inf-clojure does not yet parses out readline and/or color escapes.

@hlolli
Copy link
Contributor

hlolli commented Jun 8, 2017

I did it like this

(defun lumo-start ()
  (interactive)
  (inf-clojure "cljs-boot lumo -K -d") 
  (with-current-buffer (buffer-name)
    (split-window-sensibly) 
    (previous-window)
    (previous-buffer)))

@arichiardi
Copy link
Contributor Author

arichiardi commented Jun 8, 2017

@hlolli sorry about that, just double checking 😄 I can reproduce.

@arichiardi
Copy link
Contributor Author

You error above still happens when the first thing you do is (redu at a lumo REPL.

@arichiardi arichiardi changed the title Fix for #79 Fix for #79 and #83 Jun 14, 2017
@arichiardi
Copy link
Contributor Author

@hlolli the code above should solve things. It would be great if you can apply #81 on top and check. I did that, and I see no errors in both lumo and normal repls.

@hlolli
Copy link
Contributor

hlolli commented Jun 15, 2017

My changes for #81 are in your pull-request #82, so as expected, completions are working.

Eldoc is not on the other hand, just empty minibuffer.

But these three open pull-requests are now a soup.

@arichiardi
Copy link
Contributor Author

Oh ok I thought I took it off from mine, sorry about that! I guess @bbatsov can merge #81, than I can rebase on top of it.

@arichiardi
Copy link
Contributor Author

arichiardi commented Jun 30, 2017

The eldoc error in Lumo is related to a pr-str added somehow to the command (fixed in #84). Just discovered. I have a better patch for this, just need time to polish it up.

@arichiardi arichiardi force-pushed the fix-79 branch 2 times, most recently from ee72d95 to a152514 Compare July 2, 2017 21:46
@arichiardi arichiardi changed the title Fix for #79 and #83 Fix for #79 Jul 2, 2017
@arichiardi
Copy link
Contributor Author

Split this one in two (or more) while discovering and solving issues.

@bbatsov bbatsov closed this Jul 3, 2017
@arichiardi
Copy link
Contributor Author

@bbatsov so we are not merging this one? Why?

@bbatsov bbatsov reopened this Jul 3, 2017
@bbatsov
Copy link
Member

bbatsov commented Jul 3, 2017

My bad. Misinterpreted the comment.

@arichiardi
Copy link
Contributor Author

Ok cool lemme know 😀

@bbatsov bbatsov merged commit f666f60 into clojure-emacs:master Jul 6, 2017
@arichiardi arichiardi deleted the fix-79 branch July 6, 2017 13:30
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants