[Fix #445] def form with strings and docstrings #505

carlosgeos · 2019-01-20T23:45:13Z

I took the proposed solution here: #445 (thanks @oskarkv) and scoped it to only the def form.

Basically:

(def my_string_with_docstring "how to use" "just a string")

how to use and just a string are highlighted differently.

And:

(def im_a_string "hello")

hello is no longer highlighted as a docstring

I did what was said in these comments: #445 (comment) and #445 (comment). Now it works for defs, defprotocol, ns (whatever the form)...

The commits are consistent with our [contribution guidelines][1].
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).

carlosgeos · 2019-01-21T09:21:14Z

This also closes #469 #405 abo-abo/lispy#301

bbatsov · 2019-02-20T11:44:06Z

Please add some unit tests and update the change log as well. (This suggestions applies to the other PRs you’ve opened as well)

carlosgeos · 2019-02-20T17:30:22Z

done for this one, the others will come soon

bbatsov · 2019-02-21T22:37:05Z

test/clojure-mode-indentation-test.el

           (indent-according-to-mode)

-           (should (equal expected-state (buffer-string)))
+           (should (equal expected-state (buffer-substring-no-properties


Why is this change necessary?

Because clojure-in-docstring-p checks for a font-lock-face, indent-according-to-mode works differently (in this case) if font-lock-mode is off, so I wrote font-lock-ensure since otherwise there is no font-lock information in the temp-buffer.

If this change wasn't there, this test:

clojure-mode/test/clojure-mode-indentation-test.el

Lines 123 to 125 in 43e3e65

(check-indentation no-indent-for-def-string

"(def foo \"hello|\")"

"(def foo \"hello|\")")

would always pass, both in the current version and my version. To make it fail in the current version (which is expected and wanted), the change had to be made. I found no other way to mimic what is really happening.

And about the line you highlighted: you are right, I'll compare the strings without removing the properties, no reason to do that.

bbatsov · 2019-02-22T11:35:03Z

test/clojure-mode-indentation-test.el

@@ -68,6 +68,7 @@ values of customisable variables."
           (search-forward "|")
           (delete-char -1)
           (clojure-mode)
+           (font-lock-ensure)


How can this affect an indentation test?

I think it is because the indentation command behaves differently depending on whether font lock mode is ON or OFF, in the case of docstrings.

This branch is master with the test added: https://github.com/carlosgeos/clojure-mode/tree/temp-branch. The tests are fine, but if (font-lock-ensure) is added, the test with the docstring fails (like it should).

Hmm, that’s odd. Turning on the mode should enable the font locking.

Usually yes, but apparently not when using temp-buffer:

https://stackoverflow.com/a/23569337
https://emacs.stackexchange.com/a/44301

cichli

Thanks again! The changes look great to me. Just one question: should we still font lock the below string as a docstring?

(def foo
  "foo"
  )

It seems sexp-at-point removes that trailing whitespace, so we pass the wrong position to char-after.

carlosgeos · 2019-02-25T21:50:11Z

True, that example is incorrectly font locked. I'll make some modifications.

carlosgeos · 2019-02-25T23:24:25Z

The new solution handles whitespace and newlines better

If everything is ok, I will squash

cichli · 2019-02-25T23:27:15Z

clojure-mode.el

+                     ;; In a def, at last position is not a docstring
+                     (not (and (string= "def" firstsym)
+                               (save-excursion
+                                 (goto-char startpos)


Is this necessary given the goto-char below?

The first goto-char looks redundant but I think it is not, because there is a (sexp-at-point) below, and it needs the point at the correct position beforehand.

Also, the (sexp-at-point) form does not take any arguments so I cannot pass startpos to it.

Anyway, I tried without it and it didn't work. Happy to remove it if we find the way to make it work.

Ah, great point. I didn't think of that! Yeah, we definitely need it because of the (sexp-at-point).

Go ahead and squash if you like. I will merge once I have the rights to do so.

cichli · 2019-02-25T23:28:42Z

Apart from that one final comment, everything looks great now.

… forms - def forms can now have docstrings and strings properly font-locked - they will not be incorrectly indented added tests as well

[Fix clojure-emacs#445] def form with strings and docstrings

carlosgeos · 2019-02-27T12:44:47Z

Thanks, I also think you can close #469 and #405

carlosgeos force-pushed the fix-445 branch 2 times, most recently from 94f22b4 to 7174466 Compare February 20, 2019 17:18

bbatsov reviewed Feb 21, 2019

View reviewed changes

carlosgeos force-pushed the fix-445 branch from 43e3e65 to dcc009f Compare February 22, 2019 02:02

bbatsov reviewed Feb 22, 2019

View reviewed changes

bbatsov requested a review from cichli February 23, 2019 11:28

cichli requested changes Feb 25, 2019

View reviewed changes

cichli reviewed Feb 25, 2019

View reviewed changes

[Fix clojure-emacs#445] More accurate font locking for strings in def…

a9a0276

… forms - def forms can now have docstrings and strings properly font-locked - they will not be incorrectly indented added tests as well

carlosgeos force-pushed the fix-445 branch from f0b0ddd to a9a0276 Compare February 25, 2019 23:57

cichli added a commit to cichli/clojure-mode that referenced this pull request Feb 27, 2019

Merge pull request clojure-emacs#505 from carlosgeos/fix-445

b0b8231

[Fix clojure-emacs#445] def form with strings and docstrings

cichli merged commit a9a0276 into clojure-emacs:master Feb 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #445] def form with strings and docstrings #505

[Fix #445] def form with strings and docstrings #505

carlosgeos commented Jan 20, 2019 •

edited

Loading

carlosgeos commented Jan 21, 2019 •

edited

Loading

bbatsov commented Feb 20, 2019

carlosgeos commented Feb 20, 2019

bbatsov Feb 21, 2019

carlosgeos Feb 22, 2019 •

edited

Loading

bbatsov Feb 22, 2019

carlosgeos Feb 22, 2019

bbatsov Feb 23, 2019

carlosgeos Feb 23, 2019

cichli left a comment

carlosgeos commented Feb 25, 2019

carlosgeos commented Feb 25, 2019

cichli Feb 25, 2019

carlosgeos Feb 25, 2019

cichli Feb 25, 2019

cichli Feb 25, 2019

cichli commented Feb 25, 2019

carlosgeos commented Feb 27, 2019

	(check-indentation no-indent-for-def-string
	"(def foo \"hello\|\")"
	"(def foo \"hello\|\")")

[Fix #445] def form with strings and docstrings #505

[Fix #445] def form with strings and docstrings #505

Conversation

carlosgeos commented Jan 20, 2019 • edited Loading

carlosgeos commented Jan 21, 2019 • edited Loading

bbatsov commented Feb 20, 2019

carlosgeos commented Feb 20, 2019

Choose a reason for hiding this comment

carlosgeos Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cichli left a comment

Choose a reason for hiding this comment

carlosgeos commented Feb 25, 2019

carlosgeos commented Feb 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cichli commented Feb 25, 2019

carlosgeos commented Feb 27, 2019

carlosgeos commented Jan 20, 2019 •

edited

Loading

carlosgeos commented Jan 21, 2019 •

edited

Loading

carlosgeos Feb 22, 2019 •

edited

Loading