Skip to content

Avoid throwing an error for frequent operations like completion #125

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 1 commit into from
Jan 22, 2018

Conversation

arichiardi
Copy link
Contributor

@arichiardi arichiardi commented Jan 12, 2018

This patch introduces reduces inf-clojure-proc's responsabilities to only one:
return the processs. The new inf-clojure--proc-or-error will do both returning
and erroring.
This is necessary because while it is good to communicate an error on "eval"
operations, it is not good to have it on "under the hood" and very frequent
ones like completion and get arglists.


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)

Thanks!

@arichiardi
Copy link
Contributor Author

I also want to pass proc through all the inf-clojure-*-form functions because it seems to me a bit ugly and brittle that they get proc independently. It will take a little bit more time though so I am leaving this WIP (and try it anyways).

@arichiardi arichiardi force-pushed the improve-proc-erroring branch from 5811278 to 482826f Compare January 13, 2018 00:46
inf-clojure.el Outdated
(current-buffer)
inf-clojure-buffer)))

(defun inf-clojure--proc-or-error ()
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 a more common approach is to have just one function and some param controlling wether to raise errors or return nil. That's what we do in CIDER for instance (and I've seen in many other packages).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok will change that

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 function won't be pure anymore though

Copy link
Member

Choose a reason for hiding this comment

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

In Emacs that's totally fine. :-)

@arichiardi arichiardi force-pushed the improve-proc-erroring branch from 482826f to b13833c Compare January 21, 2018 19:36
@arichiardi arichiardi changed the title WIP - Avoid throwing an error for frequent operations like completion Avoid throwing an error for frequent operations like completion Jan 21, 2018
@arichiardi arichiardi force-pushed the improve-proc-erroring branch 2 times, most recently from 468d6cd to 6df610d Compare January 21, 2018 19:42
@bbatsov
Copy link
Member

bbatsov commented Jan 22, 2018

The changes look good, but your branch has to be rebased on top of the current master branch due to merge conflicts.

@arichiardi
Copy link
Contributor Author

I am going to do it now

This patch introduces an argument to inf-clojure-proc so that client code can
decide when to error out return the processs. This is necessary because while
it is good to communicate an error on "eval" operations, it is not good to have
it on "under the hood" and very frequent ones like completion and get arglists.
@arichiardi arichiardi force-pushed the improve-proc-erroring branch from 6df610d to a6da43a Compare January 22, 2018 20:49
@bbatsov bbatsov merged commit d3c0d4f into clojure-emacs:master Jan 22, 2018
@arichiardi arichiardi deleted the improve-proc-erroring branch January 23, 2018 03:56
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.

2 participants