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

Extend undef op to work on fully qualified symbols #670

Merged
merged 3 commits into from Mar 3, 2020

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Mar 3, 2020

Before submitting a PR make sure the following things have been done:

  • The commits are consistent with our contribution guidelines
  • You've added tests to cover your change(s)
  • All tests are passing
  • The new code is not generating reflection warnings
  • You've updated the README (if adding/changing middleware)

See clojure-emacs/cider#2814.
The current cider.el frontend sends undef op with ns as the current ns and symbol as a possibly ns-qualified var, which this PR interprets as unmapping the symbol from the corresponding other namespace.

For some reason running tests locally with make test turns up many failures in cider.nrepl.middleware.debug-integration-test which seem to be unrelated to the changes in this PR.

@bbatsov
Copy link
Member

bbatsov commented Mar 3, 2020

The current cider.el frontend sends undef op with ns as the current ns and symbol as a possibly ns-qualified var, which this PR interprets as unmapping the symbol from the corresponding other namespace.

This looks reasonable to me.
I'd suggest documenting the update behaviour in the ns docstring (high-level) and the undef function's docstring.

This should also be mentioned in the changelog.

@bbatsov bbatsov merged commit cf8dd5a into clojure-emacs:master Mar 3, 2020
@bbatsov
Copy link
Member

bbatsov commented Mar 3, 2020

Thanks for tackling this! 🙇

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.

None yet

2 participants