Skip to content

Rename ns alias improvements #556

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

Merged
merged 5 commits into from
Mar 26, 2020

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Mar 20, 2020

Some minor improvements to the command clojure-rename-ns-alias, offering minibuffer completion of existing aliases and using regexp-quote, so that namespaces containing characters like * or + can be renamed correctly without need for manual escaping.


Before submitting a PR mark the checkboxes for the items you've done (if you
think a checkbox does not apply, then leave it unchecked):

  • The commits are consistent with our [contribution guidelines][1].
  • You've added tests (if possible) to cover your change(s). Bugfix, indentation, and font-lock tests are extremely important!
  • You've run M-x checkdoc and fixed any warnings in the code you've written.
  • You've updated the changelog (if adding/changing user-visible functionality).
  • You've updated the readme (if adding/changing user-visible functionality).

@yuhan0 yuhan0 force-pushed the rename-ns-alias-improvements branch from 9edea85 to 4f9548a Compare March 20, 2020 10:43
clojure-mode.el Outdated
@@ -2684,6 +2684,19 @@ lists up."
(insert sexp)
(clojure--replace-sexps-with-bindings-and-indent)))

(defun clojure--collect-ns-aliases ()
Copy link
Member

Choose a reason for hiding this comment

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

I guess this one can even be public API and optionally take a ns form as a param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean having it as a string->list pure function? Or taking a point param marking the location of the start of the ns form?

Copy link
Member

Choose a reason for hiding this comment

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

Pure function taking a ns form as param.

clojure-mode.el Outdated
(while (re-search-forward rgx end 'noerror)
(message (match-string 1))
(push (cons (point) (match-string-no-properties 1)) res))
res)))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the value of returning cons cells here if the first item in the cons cell is not the name the alias is referring to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I wrote this a few months ago and can't remember why I had that in the first place, will simplify

Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -2,8 +2,12 @@

## master (unreleased)

### Bugs fixed
### New features
* [#556](https://github.com/clojure-emacs/clojure-mode/issues/556): `clojure-rename-ns-alias` picks up existing aliases for minibuffer completion.
Copy link
Member

Choose a reason for hiding this comment

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

Please, add a blank line after each heading.

This allows for renaming ns aliases with special regex chars like . and *,
without having to quote them
@yuhan0 yuhan0 force-pushed the rename-ns-alias-improvements branch from badf458 to 2ff902b Compare March 23, 2020 18:15
@yuhan0
Copy link
Contributor Author

yuhan0 commented Mar 23, 2020

I discovered and fixed another bug when aliases have common prefixes, eg.

[clojure.string :as string]
[clojure.spec.alpha :as s]

renaming s -> x would produce [clojure.string :as xtring]

Refactored clojure-collect-ns-aliases into a pure fn as requested, although it does seem a little un-idiomatic for elisp.

I can squash related commits together if needed, thanks

@yuhan0 yuhan0 force-pushed the rename-ns-alias-improvements branch from 2ff902b to 327f2f4 Compare March 23, 2020 18:25
@bbatsov
Copy link
Member

bbatsov commented Mar 24, 2020

Refactored clojure-collect-ns-aliases into a pure fn as requested, although it does seem a little un-idiomatic for elisp.

Looks fine to me. Add a couple of unit tests for it and we're in business.

I discovered and fixed another bug when aliases have common prefixes, eg.

Great!

I can squash related commits together if needed, thanks

That'd be ideal! Thanks!

@yuhan0 yuhan0 force-pushed the rename-ns-alias-improvements branch from 327f2f4 to 2b962d8 Compare March 24, 2020 13:23
@yuhan0
Copy link
Contributor Author

yuhan0 commented Mar 24, 2020

Okay, squashed the commits and included one test for the collect-aliases function.

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

bbatsov commented Mar 26, 2020

Thanks!

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.

2 participants