Skip to content

Fix clojure-find-ns #661

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.

Sign up for GitHub

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
Sep 7, 2023
Merged

Fix clojure-find-ns #661

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
19a2ddb
Fix clojure-find-ns
p4v4n Sep 4, 2023
48c4a67
Add comments
p4v4n Sep 4, 2023
01d8d30
Merge branch 'master' into fix-clojure-find-ns
p4v4n Sep 6, 2023
ba3f756
Cleanup
p4v4n Sep 6, 2023
06ecbc8
Add 1 more test case
p4v4n Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Merge branch 'master' into fix-clojure-find-ns
  • Loading branch information
@p4v4n
p4v4n committed Sep 6, 2023
commit 01d8d30afea1d5f6f7b7a90b22c947736c12d0a2
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## master (unreleased)

### Changes

* Improve support for multiple forms in the same line by replacing `beginning-of-defun` fn.

### Bugs fixed

* [#593](https://github.com/clojure-emacs/clojure-mode/issues/593): Fix clojure-find-ns when ns form is preceded by whitespace or inside comment form.
Expand Down
4 changes: 2 additions & 2 deletions clojure-mode.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2274,7 +2274,7 @@ This will skip over sexps that don't represent objects, so that ^hints and
"Return truthy if the first form matches FIRST-FORM."
(condition-case nil
(save-excursion
(beginning-of-defun)
(beginning-of-defun-raw)
;; Go to beginning of sexp
(clojure-forward-logical-sexp 1)
Copy link
Member

@bbatsov bbatsov Sep 4, 2023

Choose a reason for hiding this comment

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

Perhaps we should add some comments here?

Copy link
Contributor Author

@p4v4n p4v4n Sep 4, 2023

Choose a reason for hiding this comment

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

The change is to handle whitespace before comment form. Not sure if this is the best way to do it though.
Added a couple of comments now to make the intent more clear.

Copy link
Member

@bbatsov bbatsov Sep 4, 2023

Choose a reason for hiding this comment

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

My point was mainly that it looks a bit weird that we're going forward and backwards back-to-back. If that's used often in the code we can make a function I guess.

Copy link
Contributor Author

@p4v4n p4v4n Sep 4, 2023

Choose a reason for hiding this comment

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

Agreed. But I am not sure what is the correct abstraction anymore as it's more of a hack.

This is the third occurrence in the codebase but there are other places where similar logic was used with different functions and some places where edge cases are not handled. Even my current change will still break if there are multiple expressions in the same line.

Looks like all these extra steps are only needed in many places because of beginning-of-defun behaving 'incorrectly'.
For ex:

1 (ns abc) (defn abc []
             (comment
               (println "try (clojure-beginning-of-defun-function) from here"))
             "abc")

Now running (clojure-beginning-of-defun-function) or (beginning-of-defun) from any of the inner expressions takes the cursur to the beginning of the line (before 1).
The expected behaviour IMO from the name would be having the cursor before (defn.
The docstring mentions this but there is no way to easy way to control the end location of the cursor.

clojure-mode uses beginning-of-defun a lot so there are a lot of unhandled edge cases.
For example take these two code blocks:

(defn s1 [x] (str x)) (+ 1 2)

(defn s2
    [x] (str x))

(defn s1 [x] (str x)) 
(+ 1 2)
(defn s2
    [x] (str x))

Run M-x cider-eval-defun-at-point(C-c C-c) inside (+ 1 2) and the result is different in the two scenarios.

Copy link
Contributor Author

@p4v4n p4v4n Sep 4, 2023

Choose a reason for hiding this comment

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

tldr: IIUC bad choice of abstraction from beginning-of-defun fn has propagated many edge cases possibly in all lisp modes.
Maybe I am missing something basic or someone already solved this issue.

Copy link
Contributor Author

@p4v4n p4v4n Sep 5, 2023

Choose a reason for hiding this comment

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

@bbatsov I have raised a separate PR for replacing beginning-of-defun fn completely here.
#663
After merging that PR these two lines here can be removed.

Copy link
Member

@bbatsov bbatsov Sep 5, 2023

Choose a reason for hiding this comment

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

Thanks for diving deep into this!

Copy link
Contributor Author

@p4v4n p4v4n Sep 6, 2023

Choose a reason for hiding this comment

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

Merged latest master and removed the extra lines here.
Will wait for @vemv to resolve the other conversation.

(clojure-backward-logical-sexp 1)
Expand Down Expand Up @@ -3228,7 +3228,7 @@ With universal argument \\[universal-argument], act on the \"top-level\" form."
(beginning-of-defun-raw)
(clojure--toggle-ignore-next-sexp)))


;;; ClojureScript
(defconst clojurescript-font-lock-keywords
(eval-when-compile
Expand Down
You are viewing a condensed version of this merge commit. You can view the full changes here.