Skip to content

Pass :always-return-ns-form true to clean-ns #558

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

Conversation

DerGuteMoritz
Copy link
Contributor

This allows for cleaning up whitespace even if there are no structural changes.

Note that I went with always replacing the ns form even if there were no whitespace changes because there's no function for only extracting the current ns form for comparison (in other words: there's only clojure-delete-and-extract-sexp but no clojure-extract-sexp). It's probably fine!

Depends on clojure-emacs/refactor-nrepl#407 and a corresponding update of cljr-injected-middleware-version once released.

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • The new code is not generating byte compile warnings (run cask exec emacs -batch -Q -L . -eval "(progn (setq byte-compile-error-on-warn t) (batch-byte-compile))" clj-refactor.el)
  • All tests are passing (run ./run-tests.sh)
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)

This allows for cleaning up whitespace even if there are no structural changes.
Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

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

Thanks again!

Note that I went with always replacing the ns form even if there were no whitespace changes because there's no function for only extracting the current ns form for comparison

Unfortunately this doesn't seem ideal as it can needlessly touch the underlying file - might be wasteful for misc other tools.

Note that there's defun cljr--maybe-eval-ns-form. Jumping to definition from there you can find inspiration.

Cheers - V

CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

## Unreleased

- `clean-ns` will reformat `ns` form even when no structural changes were made.
Copy link
Member

Choose a reason for hiding this comment

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

Please expand on this so that ppl can see its usefulness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about now?

That is, only if its string representation has changed.
@DerGuteMoritz
Copy link
Contributor Author

Unfortunately this doesn't seem ideal as it can needlessly touch the underlying file - might be wasteful for misc other tools.

Ah yes, point taken!

Note that there's defun cljr--maybe-eval-ns-form. Jumping to definition from there you can find inspiration.

Took clojure-delete-and-extract-sexp as inspiration instead. Not too happy about the string-trim-right which is necessary because the string returned by refactor-nrepl comes with a trailing newline. Could of course also be changed on that end but oh well!

@vemv
Copy link
Member

vemv commented Mar 1, 2024

Very nice!

LGTM.

Let me release refactor-nrepl first so that merging this will work.

@vemv
Copy link
Member

vemv commented Mar 2, 2024

I've released https://clojars.org/refactor-nrepl/versions/3.10.0

Please cherry-pick this commit 282a43c and test out the changes for about a week!

Cheers - V

@DerGuteMoritz
Copy link
Contributor Author

Works as intended so far 👍

@vemv vemv merged commit dc1bbc8 into clojure-emacs:master Mar 10, 2024
@vemv
Copy link
Member

vemv commented Mar 10, 2024

Thanks much!

I've cut 3.12.0, to be melpa-visible in a bit.

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