Skip to content

Request: variable to control cljr--clj-context-p popup #394

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

Closed
eggsyntax opened this issue Oct 22, 2017 · 4 comments
Closed

Request: variable to control cljr--clj-context-p popup #394

eggsyntax opened this issue Oct 22, 2017 · 4 comments

Comments

@eggsyntax
Copy link
Contributor

Hi, let me first say that I find clj-refactor incredibly helpful and make use of it every day. Thanks very much for such a great tool!

I have a fairly minor request: clj-refactor.el contains the following function:

(defun cljr--clj-context-p ()
  "Is point in a clj context?"
  (or (cljr--clj-file-p)
      (when (cljr--cljc-file-p)
        (if (cljr--point-in-reader-conditional-p)
            (cljr--point-in-reader-conditional-branch-p :clj)
          (string-equal (cljr--prompt-user-for "Language context at point? "
                                               (list "clj" "cljs"))
                        "clj")))))

Unlike pretty much everything else in clj-refactor, I find this more annoying than helpful; I nearly always escape out of the popup -- often after typing half the fn/var name I want, meaning I have to retype it once I realize I'm in the popup. I've taken a look to see if I saw a variable that controls this, but I haven't found one.

Can we have a user-settable variable that controls this, such that the popup is skipped if the variable is set to false? It could result in the same behavior that escaping out of the popup does (even if there's a bit of cost to clj-refactor's ability to help out in that case). Of course I'm imagining it set to true by default, so that there's no behavior change unless the user explicitly sets the variable in their user config.

I'm happy to try to put together a PR if that would be helpful.

Thanks!

@expez
Copy link
Member

expez commented Oct 22, 2017

Hi

Sorry you're having a bad time! Unfortunately, the cljs/cljc support isn't that great because I (and I think @benedekfazekas too) only write clj at work. This means we're especially grateful when some of you speak up! :)

Looking at the function one layer out:

(defun cljr--get-aliases-from-middleware ()
  (when-let (aliases (cljr--call-middleware-for-namespace-aliases))
    (if (cljr--clj-context-p)
        (gethash :clj aliases)
      (gethash :cljs aliases))))

I think perhaps we're doing the wrong thing here. In my eyes we should probably:

  1. Look for clj context
  2. If not found, look for cljs context
  3. If not found fall back to that prompt you've come to hate.

Does that sound better to you?

If this doesn't solve the problem then I'm OK with adding a variable too. The current situation sounds awful :(

I'm happy to try to put together a PR if that would be helpful.

That would be lovely!

Thanks for caring!

@eggsyntax
Copy link
Contributor Author

Oh, no, don't get me wrong! It's FAR from awful, it's just a minor annoyance. Barely even puts a dent in the huge value I get from clj-refactor :)

I've only had time to take a quick look back, but -- offhand, I'm not sure your approach would solve this problem. It only happens in cljc files (which I see I completely failed to mention in my OP). The code handles the case when we're inside a reader conditional, but being outside of reader conditionals triggers the prompt. So I'm not sure it's possible to resolve to clj or cljs context (unless there's a strategy I'm unaware of).

@expez
Copy link
Member

expez commented Oct 23, 2017

I see. So we need a variable that disables the magic require feature when the dialect can't be contextually resolved (to avoid prompting the user). Is that right?

That variable is going to be hard to name!

@eggsyntax
Copy link
Contributor Author

Yeah, that's what I was thinking.

bypass-context-popup, maybe?
or... bypass-dialect-popup?

I'm definitely hoping to find time to get a PR together, but it'll probably be 1-2 weeks; don't hesitate to change it before then if you get inspired.

Thanks again!

expez pushed a commit that referenced this issue Nov 17, 2017
When the user enters an apparent namespace like `somelib/`, and the
language context is ambiguous, clj-refactor creates a popup asking the
user to specify whether the context is "clj" or "cljs".

This change allows the user to specify a language context which should
be assumed, so that clj-refactor will choose that context in ambiguous
cases rather than creating a popup.

Default behavior is still to create the popup; this change only
creates a variable that lets the user override that behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants