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

Rename ns aliases in selected region #590

Merged
merged 5 commits into from
May 2, 2021

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Apr 21, 2021

When copying code from other sources, the conventions for aliasing namespaces may be different (eg. str vs string for clojure.string) and one has to manually search and replace these aliases.

Since there was already a clojure-rename-ns-alias command with most of the relevant logic, I extended it to be able to rename aliases in a selected region. In that case, it will not touch the ns form at all and only pick up aliases from the region.

I also cleaned up the code a bit from my previous PR here, moving the ns-alias related functions together and renaming clojure-collect-ns-aliases with double dashes (there was really no reason for it to be public in the first place).

The API could be changed to introduce another command instead of doing region-active DWIM on clojure-rename-ns-alias. But in practice I find that such renaming usually involves multiple (selected) forms, so a command that works on top-level forms probably isn't that useful.

Will update the changelog and readme after details are confirmed :)

Thanks!


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.
  • 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).

Thanks!

@yuhan0 yuhan0 force-pushed the rename-ns-region branch from ffa78ec to b3a1e23 Compare April 21, 2021 22:09
@bbatsov
Copy link
Member

bbatsov commented May 2, 2021

The API could be changed to introduce another command instead of doing region-active DWIM on clojure-rename-ns-alias. But in practice I find that such renaming usually involves multiple (selected) forms, so a command that works on top-level forms probably isn't that useful.

It's fine in its current form.

I like the change, but it'd be nice to advertise this functionality somewhere (e.g. in the README), otherwise I doubt many people will figure out there's special handling for regions.

@yuhan0
Copy link
Contributor Author

yuhan0 commented May 2, 2021

Having used the command in some major refactorings over the past week, I also made another usability change to make it easier to rename multiple aliases at one go.
An animated GIF would be nice to illustrate the new functionality, I'm not sure if @anthonygalea would mind recording one to keep in the style of the rest of the page :)

@bbatsov bbatsov merged commit 512e285 into clojure-emacs:master May 2, 2021
@bbatsov
Copy link
Member

bbatsov commented May 2, 2021

Yeah, that'd be nice indeed, but I think we're good to merge regardless. Thanks for tackling this!

@anthonygalea
Copy link
Contributor

Thank you for this change @yuhan0 👍 (and also for asking for the recording, happy to help)

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.

3 participants