Skip to content

Introduce threading refactoring commands #89

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 1 commit into from
Apr 30, 2025

Conversation

rrudakov
Copy link
Contributor

@rrudakov rrudakov commented Apr 29, 2025

All related tests from clojure-mode pass except of those which "unjoin" lines, in clojure-mode it's implemented via text properties and doesn't work for the same expression after restarting Emacs. If it's an important feature, we can deal with that later.

I also fixed a couple of small issues with indentation and aligning forms.


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!

@rrudakov rrudakov closed this Apr 29, 2025
@rrudakov rrudakov force-pushed the feature/thread-first-last branch from 50c8809 to d9ae5fc Compare April 29, 2025 12:16
@rrudakov rrudakov reopened this Apr 29, 2025
@rrudakov
Copy link
Contributor Author

Weird, I didn't close it. I guess GitHub did it somehow :)

(and (clojure-ts--list-node-p second-child)
(> (treesit-node-child-count second-child t) 1))))

(defun clojure-ts-thread ()
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this raise some error if the form at point is not threadable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In clojure-ts-mode it just returns nil, I did the same here. Maybe because it's used in a loop in both thread-first-all and thread-last-all?

Copy link
Member

Choose a reason for hiding this comment

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

Much of the refactoring code was copied from clj-refactor in bulk early on, so I didn't pay close attention to each command there - that's why I notice some stuff for the first time.

I think it probably makes sense to have an optional param controlling whether to raise an error or return nil, and perhaps even introduce some lower-level function that's not actually command that all of the commands call. (extracted from clojure-ts-thread)

Any rate - what you're doing right now is a great opportunity for us to examine closely each refactoring command and make them slightly better here and there in the process of porting them to clojure-ts-mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I already discovered a few issues in clojure-mode commands and fixed them in clojure-ts-mode implementation.

I updated this function, it signals user-error now if called interactively and there is no threading form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it probably makes sense to have an optional param controlling whether to raise an error or return nil, and perhaps even introduce some lower-level function that's not actually command that all of the commands call. (extracted from clojure-ts-thread)

Perhaps we could look into it when we port more refactoring commands and see some pattern that can be extracted.

@bbatsov
Copy link
Member

bbatsov commented Apr 29, 2025

All related tests from clojure-mode pass except of those which "unjoin" lines, in clojure-mode it's implemented via text properties and doesn't work for the same expression after restarting Emacs. If it's an important feature, we can deal with that later.

I wouldn't say that's particularly important.

@rrudakov rrudakov force-pushed the feature/thread-first-last branch from d9ae5fc to 63ab3d9 Compare April 29, 2025 13:09
@rrudakov rrudakov requested a review from bbatsov April 29, 2025 13:10
@rrudakov rrudakov force-pushed the feature/thread-first-last branch from 63ab3d9 to aace501 Compare April 29, 2025 13:43
@rrudakov rrudakov force-pushed the feature/thread-first-last branch from aace501 to 2fcc5c7 Compare April 29, 2025 14:54
@bbatsov bbatsov merged commit 3569c90 into clojure-emacs:main Apr 30, 2025
3 checks passed
@rrudakov rrudakov deleted the feature/thread-first-last branch April 30, 2025 16:46
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