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

cider-undef does not unmap vars from other namespaces #2814

Closed
yuhan0 opened this issue Mar 2, 2020 · 4 comments
Closed

cider-undef does not unmap vars from other namespaces #2814

yuhan0 opened this issue Mar 2, 2020 · 4 comments
Labels
bug good first issue A simple tasks suitable for first-time contributors

Comments

@yuhan0
Copy link
Contributor

yuhan0 commented Mar 2, 2020

Expected behavior

Calling cider-undef on a fully qualified symbol foo/bar should unmap the var from the corresponding (aliased) foo namespace.

Actual behavior

The var is not undefined (fails silently)

Steps to reproduce the problem

my/lib.clj

(ns my.lib)
(def abc 123)

my/app.clj

(ns my.app
  (:require [my.lib :as lib])

lib/abc

In the my/app.clj buffer, call M-x cider-undef with point on lib/abc.
Observe: the var is still accessible after the command.

Environment & Version information

CIDER version information

;; CIDER 0.25.0snapshot (package: 20190708.1036), nREPL 0.6.0
;; Clojure 1.10.1, Java 13.0.1

Emacs version

26.3

Operating system

macOS 10.15

@yuhan0
Copy link
Contributor Author

yuhan0 commented Mar 2, 2020

This should be a relatively straightforward fix I'm willing to tackle, but it's not clear whether it should be the responsibility of cider or cider-nrepl.

Option 1:

cider.el ensures that ns and symbol params are simple symbols

  • cider-undef sees lib/abc
    • uses regex to split it into "lib" and "abc",
    • calls (cider-resolve-alias "lib") => "my.lib",
    • and sends the message ("op" "undef" "ns" "my.lib" "symbol" "abc")

Option 2:

cider-nrepl library does the decoding:

  • cider-undef sends the message ("op" "undef" "ns" "my.app" "symbol" "lib/abc")
  • cider.nrepl.middleware.undef/undef notices that (symbol "lib/abc") is fully qualified
    • calls namespace, name to get "lib" and "abc"
    • (get (ns-aliases "my.app") "lib") => "my.lib"
    • (ns-unmap "my.lib" "abc")

@bbatsov
Copy link
Member

bbatsov commented Mar 2, 2020

Probably option 2 would be more efficient, as you need to call the undef op anyways.

@bbatsov bbatsov added bug good first issue A simple tasks suitable for first-time contributors labels Mar 2, 2020
@yuhan0
Copy link
Contributor Author

yuhan0 commented Mar 2, 2020

Do you mean the resolve-alias op? Both options are calling the same undef op, I don't really think it's a matter of efficiency but whether to narrow the message spec vs. broaden the input parsing on the backend.

@bbatsov
Copy link
Member

bbatsov commented Mar 3, 2020

I'm puzzled. There's no resolve-alias op. My main point was that if the middleware is smarter this automatically benefits all clients using it.

@yuhan0 yuhan0 closed this as completed Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue A simple tasks suitable for first-time contributors
Projects
None yet
Development

No branches or pull requests

2 participants