Skip to content

[Fix #537] Fail early in cljr-rename-symbol if narrowing is in effect #538

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

Conversation

rechvs
Copy link
Contributor

@rechvs rechvs commented Jan 17, 2023

Previously the function would report a successful renaming without actually having renamed anything (see #537).

@rechvs rechvs changed the title [Fix #537] Fail early in cljr-rename-symbol if narrowing is in effect [Fix #537](https://github.com/clojure-emacs/clj-refactor.el/issues/537) Fail early in cljr-rename-symbol if narrowing is in effect Jan 17, 2023
@rechvs rechvs changed the title [Fix #537](https://github.com/clojure-emacs/clj-refactor.el/issues/537) Fail early in cljr-rename-symbol if narrowing is in effect [Fix #537] Fail early in cljr-rename-symbol if narrowing is in effect Jan 17, 2023
@rechvs
Copy link
Contributor Author

rechvs commented Jan 17, 2023

I tried to add a test, but make test results in

cask exec buttercup -L .
Warning (emacs):
clj-refactor.el:78: First line is not a complete sentence
...
Failing due to checkdoc warnings...
make: *** [Makefile:35: unit-tests] Error 1

even when executed in the master branch, so I had no way of knowing whether my test would have actually worked. Is this expected? Or am I misinterpreting the output?
(Taking the risk of stating the obvious here:) It would be nice if the CircleCI checks would work again, so I could compare my local setup with the CI one to find out what’s causing the tests to fail on my machine.

Nevertheless thanks a lot for your work on this project. ❤️ After working with IntellIJ IDEA for quite some time now, I’ve really grown fond of its refactoring capabilities, so cljr-rename-symbol in particular is a real godsend.

@rechvs rechvs marked this pull request as ready for review January 17, 2023 17:18
@vemv
Copy link
Member

vemv commented Jan 18, 2023

Thanks much!

I'll check what's going on with CI as soon as I have the chance.

@bbatsov
Copy link
Member

bbatsov commented Feb 2, 2023

Won't it be better to just ignore the narrowing instead?

@rechvs
Copy link
Contributor Author

rechvs commented Feb 21, 2023

Won't it be better to just ignore the narrowing instead?

As I said in #537: that would also be my preferred option. But I’m of course in no position to decide whether that’s the way forward.

Renke Christian von Seggern added 2 commits February 21, 2023 14:10
… is in effect

Previously the function would report a successful renaming without
actually having renamed anything.
Otherwise the linter will complain about the "if" missing its "else"
branch.
@rechvs rechvs force-pushed the 537-cljr-rename-symbol-not-working-properly-when-narrowing-is-in-effect branch from 6597a2e to 456b8aa Compare February 21, 2023 13:12
@vemv vemv closed this in c63dfa9 Jul 4, 2023
@vemv
Copy link
Member

vemv commented Jul 4, 2023

This commit is into the 3.7.0 release and will be visible in MELPA/etc within a couple hours.

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.

4 participants