Fix clojure-find-ns

[p4v4n]

• when ns is preceded by whitespace or inside comment form.
• closes clojure-find-ns breaks if the ns form is preceded by whitespace #593

[bbatsov]

Perhaps we should add some comments here?

[p4v4n]

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.

[bbatsov]

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.

[p4v4n]

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.

[p4v4n]

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.

[p4v4n]

@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.

[bbatsov]

Thanks for diving deep into this!

[p4v4n]

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

[vemv]

Nice work!

The other day I tried to fix a similar issue but my fix broke other things. Could you check if adding these these tests would also pass?

d6a43ee

I was particularly interested in this case:

1 (ns foo)

however, if it's too hard of a challenge (as it was for me), feel free to leave it.

[p4v4n]

Nice work!

The other day I tried to fix a similar issue but my fix broke other things. Could you check if adding these these tests would also pass?

d6a43ee

I was particularly interested in this case:

1 (ns foo)

however, if it's too hard of a challenge (as it was for me), feel free to leave it.

@vemv Interesting. I haven't thought about this case before. Your change to the regex is more correct as it will allow any form before the comment form.
I have tried it and only 1 test case seems to be failing.
(with-clojure-buffer "(ns ^{:foo {:bar :baz} :fake (ns in.meta)} foo \"docstring (ns misleading)\")" (expect (clojure-find-ns) :to-equal "foo"))

Fixing it properly requires making clojure-namespace-regexp more sophisticated to not match eagerly. There is already such a regex in the code clojure-namespace-name-regex but it is now obsolete.I don't have context on why it was made obsolete.

A simpler fix would be take your regex change and delete this unlikely test case! Allowing any forms before comment is far more likely scenario than having fake ns in metadata.

[bbatsov]

A simpler fix would be take your regex change and delete this unlikely test case! Allowing any forms before comment is far more likely scenario than having fake ns in metadata.

I agree.

[bbatsov]

Btw, you'll also have to rebase this on top of master.

[p4v4n]

Btw, you'll also have to rebase this on top of master.

Already merged latest master. The commits should be squashed here so didn't bother with a rebase.

[vemv]

@p4v4n thanks much for looking into that!

I'd be fine with no action for the time being. Perhaps, a year from now most people will be using clojure-ts-mode, which since isn't regex-based, shouldn't be as prone to false positives.

[vemv]

LGTM after a couple minor suggestions

[vemv]

Suggested change

	(expect (clojure-find-ns) :to-equal "bar"))
	)
	(expect (clojure-find-ns) :to-equal "bar")))

[vemv]

A simpler fix would be take your regex change and delete this unlikely test case! Allowing any forms before comment is far more likely scenario than having fake ns in metadata.

I agree.

When put like this, I actually agree too. (I could as well flip a coin tbh). Feel free to tackle it in a follow-up PR.

[p4v4n]

When put like this, I actually agree too. (I could as well flip a coin tbh). Feel free to tackle it in a follow-up PR.

I was partially joking about deleting the test case. My past experiences with emacs lisp regexes were not very pleasant.(I haven't used the rx macros though TBF)

Will work on it in a separate branch.We can go with the compromise solution if the proper regex is difficult.