Identation issues in docstrings

[expez]

(ns test.core
  "If I write a really long line without worrying about indentation
  and then call clojure-fill-paragraph to indent once I'm done it
  looks like this and all is well.

This paragraph on the other hand was created by hitting RET to indent
  manually.  Every time I do this the indentation moves all the way to
  the left, instead of staying aligned.  Calling clojure-fill-paragraph
  now will indent all but the first line in this paragraph properly.")

[bbatsov]

@bzg is working on this particular feature.

[bzg]

Thanks for the heads up.

I'll have a look but can't do it before next week.

[bbatsov]

@bzg No worries. Now that we have some unit tests you might add a few for the docstring filling logic as well.

[expez]

I also think it would be better if the first char on the second line was aligned with the first char on the first line and not with the ". Like this:

(defn foo []
  "Alignment
   like this")

(defn bar []
  "Instead
  of this")

This would make it consistent with how the content of a [ ] is indented. E.g. we do this:

(let [foo (bar)
      baz (atom nil)]
...)

and not this, where baz is aligned with [ and not foo:

(let [foo (bar)
     baz (atom nil)]
...)

Thoughts?

[bzg]

@expez The newline/(auto)indentation is tricky in Emacs. I push a PR (https://github.com/clojure-emacs/clojure-mode/pull/243) so that TAB does the right thing in docstring, but there is no auto-indentation yet. Will have a closer look.

As for whether this should be

"a docstring... and some more text

or

"a docstring and some more text"

I don't know. Aesthetically and for consistency with the rest of Clojure indentation I agree, but all the functions in Clojure source code use the "wrong" indentation. The question should be raised on the Clojure mailing list so that everyone can agree on the same convention, don't you think so?

[expez]

Every function looks like that in the Clojure source because that's what happens when you hit M-q when writing lisp source. Nobody is going to manually indent and expect everyone else to do the same.

That might be an argument in and of itself, though, that this is what you get in emacs lisp or common lisp mode, in emacs, when you hit M-q and since clojure is a lisp we should conform.

The clojure plugin for vim already has a setting, which is off by default, called clojure_align_multiline_strings which does what I propose. Together these two editors have 75% market share so we won't be inconveniencing anyone very much if we change this default, and should in fact expect other less popular editors to conform, especially if the style guide @bbatsov is maintaining is updated. The fact that the vim plugin already has a setting for this indicates that a few people have already thought about this. In fact, the default in Vim might reflect them conforming to us (or rather most existing source code), as most users used Emacs for writing clojure in the very beginning.

[arrdem]

@expez the problem with this example you present...

(defn foo []
  "Alignment
   like this")

(defn bar []
  "Instead
  of this")

is that clojure.repl/doc, Grimoire and other docstring access/manipulation tools account for that missing 1st space. Implementing the 2nd behavior would break existing formatting schemes for no good reason. I realized this behavior existed recently, as my docstring style had been exactly what you propose

(defn f
  "(λ x) → ⊤
   (λ x → y) → ⊤"
  ([x])
  ([x y]))

which exhibits exactly this mis-alignment behavior.

user> (defn f
  "(λ x) → ⊤
   (λ x → y) → ⊤"
  ([x])
  ([x y]))

#'user/f
user> (use 'clojure.repl)
nil
user> (doc f)
-------------------------
user/f
([x] [x y])
  (λ x) → ⊤
   (λ x → y) → ⊤
nil

So no, I don't think that the 75% market share of emacs and vim together justifies this formatting breaking change. Otherwise :thumbsup: on this, the first line failure to indent really gets on my nerves.

[bzg]

I created this PR (https://github.com/clojure-emacs/clojure-mode/pull/244) which adds an option for this.

[expez]

Thanks for pointing that out, @arrdem!

If this doesn't lead to actual breakage in the tools, just moving the alignment problem from the source to the output of the tools, then I'll start using the new default @bzg created--at least for my personal use--and hope it catches on. That way, in a few years, all tools might possibly take this into account and we can be free of this problem. I don't want to wait until the next lisp to fix this :)

[bbatsov]

@arrdem I guess we should simply patch doc to handle both styles. The existing default seems pretty odd to me.

[arrdem]

@bbatsov is your issue with the implicit two spaces, or that it should be an implicit three spaces or what? The standing behavior seems pretty reasonable to me. I certainly don't want Clojure to try and package a "real" typesetting engine for docstrings :tongue:

[genovese]

I am having a persistent but somewhat different problem with docstring indentation. Depending on the content of the docstring (in particular, punctuation), the second line of a single-paragraph docstring is not indented. The code example below illustrates the problem and show that it only arises in particular circumstances. One of the four functions below fills (its docstring) incorrectly, the other three -- with only slightly varied docstrings -- fill correctly. The results of the filling are given at the end for each case. I haven't had a chance to trace this problem through the code yet, but I thought I'd submit the examples. The comment in the code gives details of the context.

;;; Using M-q inside each of the following three docstrings gives different
;;; behavior, with the second incorrect. Note the period at the end of the 
;;; first line makes the difference, though it depends on the line length as 
;;; the third example shows. But if the period is there and it starts filled 
;;; properly, it stays filled properly, as the fourth example shows.
;;;
;;; platform:             Mac OS X 10.7.5
;;; emacs-version:        Gnu Emacs 24.4.1 (installed by homebrew)
;;; clojure-mode-version: 3.1.0-snapshot
;;;
;;; paragraph regexps in Emacs set at initialization time by:
;;; (setq paragraph-start paragraph-separate            ; Use blank lines to separate paragraphs by default
;;;       adaptive-fill-regexp "[ \t]*\\([>*%#]+ +\\)?" ; Fill around comment beginnings and yanked-messages
;;;       sentence-end-double-space nil                 ; Allow frenchspacing
;;;       page-delimiter "^\\(\f\\|\n\n+\\)")           ; FF or 2+ consecutive blank lines
;;;
;;; Resetting paragraph-start to its original value has no effect.
;;;
;;; fill-prefix is nil and fill-column is 72. Changing the latter has no effect.
;;; 
;;; Filling of these examples works fine in analogous defuns in elisp and CL.

(defn fills-right
  "Lorem ipsum dolor sit amet, consectetur adipiscing elit sed do ZZZ
eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim
ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut
  aliquip ex ea commodo consequat. Duis aute irure dolor in
  reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla
  pariatur. Excepteur sint occaecat cupidatat non proident, sunt in
  culpa qui officia deserunt mollit anim id est laborum."
  []
  true)

(defn fills-wrong
  "Lorem ipsum dolor sit amet, consectetur adipiscing elit sed do ZZZ.
eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim
ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut
  aliquip ex ea commodo consequat. Duis aute irure dolor in
  reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla
  pariatur. Excepteur sint occaecat cupidatat non proident, sunt in
  culpa qui officia deserunt mollit anim id est laborum."
  []
  true)

(defn fills-right-too
  "Lorem ipsum dolor sit amet, consectetur adipiscing. 
elit sed do ZZZ
eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim
ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut
  aliquip ex ea commodo consequat. Duis aute irure dolor in
  reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla
  pariatur. Excepteur sint occaecat cupidatat non proident, sunt in
  culpa qui officia deserunt mollit anim id est laborum."
  []
  true)

(defn fills-right-stays-filled
  "Lorem ipsum dolor sit amet, consectetur adipiscing elit sed do ZZZ.
  eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad
  minim veniam, quis nostrud exercitation ullamco laboris nisi ut
  aliquip ex ea commodo consequat. Duis aute irure dolor in
  reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla
  pariatur. Excepteur sint occaecat cupidatat non proident, sunt in
  culpa qui officia deserunt mollit anim id est laborum."
  []
  true)

;;; Results of filling in these three examples, M-q with point in docstring

(defn fills-right-filled
  "Lorem ipsum dolor sit amet, consectetur adipiscing elit sed do ZZZ
  eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad
  minim veniam, quis nostrud exercitation ullamco laboris nisi ut
  aliquip ex ea commodo consequat. Duis aute irure dolor in
  reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla
  pariatur. Excepteur sint occaecat cupidatat non proident, sunt in
  culpa qui officia deserunt mollit anim id est laborum."
  []
  true)

(defn fills-wrong-filled
  "Lorem ipsum dolor sit amet, consectetur adipiscing elit sed do ZZZ.
eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad
  minim veniam, quis nostrud exercitation ullamco laboris nisi ut
  aliquip ex ea commodo consequat. Duis aute irure dolor in
  reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla
  pariatur. Excepteur sint occaecat cupidatat non proident, sunt in
  culpa qui officia deserunt mollit anim id est laborum."
  []
  true)

(defn fills-right-too-filled
  "Lorem ipsum dolor sit amet, consectetur adipiscing. elit sed do ZZZ
  eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad
  minim veniam, quis nostrud exercitation ullamco laboris nisi ut
  aliquip ex ea commodo consequat. Duis aute irure dolor in
  reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla
  pariatur. Excepteur sint occaecat cupidatat non proident, sunt in
  culpa qui officia deserunt mollit anim id est laborum."
  []
  true)

[bzg]

Hi Christopher,

Christopher Genovese notifications@github.com writes:

I am having a persistent but somewhat different problem with docstring indentation.

Sorry, I won't have time to check this particular issue before long, I hope someone else will.

[genovese]

Thanks, Bastien.

As an update, I have a simpler explanation for what is happening. Whenever the first line of a doc-string ends on a period or comma (with no subsequent spaces), the indentation of the second line is not changed.

This explains all the examples I showed because in fills-right-too, I inadvertently left some spaces and in fills-right-stays-filled, the second line is where it should be.

In clojure-fill-paragraph, the variable paragraph-separate has added a regex that includes a line with optional spaces, quote, and ending with a period or comma, and I wonder if this is interacting with paragraph-start in such a way to cause the problem. I'll try to look into it further.

[bzg]

Christopher Genovese notifications@github.com writes:

I'll try to look into it further.

Thanks in advance for that. Please don't hesitate to provide experimental patches, it helps moving forward.

[genovese]

Got busy at work; sorry for the delay.

I've attached (and included below) a small patch (based on the current 4.0.0 clojure-mode.el) that changes a couple lines of the function clojure-fill-paragraph (line 288) that deal with docstring filling.

I believe the problem lies in the setting of paragraph-start and paragraph-separate. The current setting violates the documented invariant in which paragraph-start matches separators and starts, and it can cause the filling to ignore the first line and start the second line without the fill-prefix. This is what caused the cases I reported, and the problem manifests itself in a few other ways as well.

I made three changes: 1. I eliminated the setting of paragraph-separate,

2. I moved the new regex into paragraph-start where it conceptually belongs anyway, and 3. I added a brace to the last part of the regex so that filling would recognize a metadata map after a docstring to not be part of the comment, which the current function misses.

I've generated a number of ad hoc test cases and grabbed a variety of code in the wild (from well-known github repositories) to compare both versions of docstring filling. The new version looked good in all these cases, so I'm optimistic. But I know that it's not a comprehensive test suite.

So do take a look if you get a chance, and let me know if you want more (or particular) tests. I have always found the ways of paragraph-start and paragraph-separate to be mysterious ones. Let me know what you think.

Thanks and regards, Chris

* clojure-mode.el Tue Dec 16 20:55:18 2014 --- alt-clojure-mode.el Tue Dec 16 21:14:30 2014 ***** If JUSTIFY is non-nil, justify as well a * 292,300 ** (if (clojure-in-docstring-p) (let ((paragraph-start (concat paragraph-start ! "|\s-([(;:\"[]|~@|`(|#'()")) ! (paragraph-separate ! (concat paragraph-separate "|\s-\".[,.]$")) (fill-column (or clojure-docstring-fill-column fill-column)) (fill-prefix (clojure-docstring-fill-prefix))) (fill-paragraph justify)) --- 292,298 ---- (if (clojure-in-docstring-p) (let ((paragraph-start (concat paragraph-start ! "|\s-\".[,.]$|\s-([(;:\"{[]|~@|`(|#'()")) (fill-column (or clojure-docstring-fill-column fill-column)) (fill-prefix (clojure-docstring-fill-prefix))) (fill-paragraph justify))

On Sat, Dec 6, 2014 at 10:25 AM, Bastien notifications@github.com wrote:

Christopher Genovese notifications@github.com writes:

I'll try to look into it further.

Thanks in advance for that. Please don't hesitate to provide experimental patches, it helps moving forward.

— Reply to this email directly or view it on GitHub https://github.com/clojure-emacs/clojure-mode/issues/241#issuecomment-65901413 .

[genovese]

The patch didn't come through here well, so here is a gist with the patch. I've also included it more cleanly below.

*** clojure-mode.el Tue Dec 16 20:55:18 2014
--- alt-clojure-mode.el Tue Dec 16 21:14:30 2014
*************** If JUSTIFY is non-nil, justify as well a
*** 292,300 ****
    (if (clojure-in-docstring-p)
        (let ((paragraph-start
               (concat paragraph-start
!                      "\\|\\s-*\\([(;:\"[]\\|~@\\|`(\\|#'(\\)"))
!             (paragraph-separate
!              (concat paragraph-separate "\\|\\s-*\".*[,\\.]$"))
              (fill-column (or clojure-docstring-fill-column fill-column))
              (fill-prefix (clojure-docstring-fill-prefix)))
          (fill-paragraph justify))
--- 292,298 ----
    (if (clojure-in-docstring-p)
        (let ((paragraph-start
               (concat paragraph-start
!                      "\\|\\s-*\".*[,\\.]$\\|\\s-*\\([(;:\"{[]\\|~@\\|`(\\|#'(\\)"))
              (fill-column (or clojure-docstring-fill-column fill-column))
              (fill-prefix (clojure-docstring-fill-prefix)))
          (fill-paragraph justify))

[bzg]

Thanks Christopher, I'll have a look when I have a moment, don't hold your breath though.

[genovese]

Bastien,

If I had a choice, then, given your timing, I would suggest making the change. We know that the current code has two problems (the filling failure and the metadata-map handling), that the revised code fixes both problems, and that over at least a reasonably representative sample of docstrings the revised code causes no new problems. The change is small and it and its effect are localized, so the downside risk is minimal.

It's up to you, of course. If there's something more you'd like me to do, let me know.

Thanks, Chris

On Wed, Dec 17, 2014 at 10:12 AM, Bastien notifications@github.com wrote:

Thanks Christopher, I'll have a look when I have a moment, don't hold your breath though.

— Reply to this email directly or view it on GitHub https://github.com/clojure-emacs/clojure-mode/issues/241#issuecomment-67335723 .

[tsdh]

@bzg The problem I have with the current clojure-indent-line function is that I frequently have example code and ASCII art in docstrings which shall not be touched. E.g., I have

(defn foo
  "foo should be called like

     (with-bar something
       (foo x))

  where something is something."
  [x]
  (do-stuff-with m))

and after indentation I end up with

(defn foo
  "foo should be called like

  (with-bar something
  (foo x))

  where something is something."
  [x]
  (do-stuff-with m))

Of course, now you'll say "then don't hit TAB in docstrings" but I use aggressive-indent-mode which re-indents while typing.

Right now, I simply fset clojure-indent-line to lisp-indent-line which returns 'noindent for strings. That solves my issue but I don't think that forcing every line in a docstring to start with the same number of spaces is a good feature in general. Maybe adding the clojure fill prefix in case there are less spaces would be ok, but removing spaces a user has intentionally added is not IMHO.

[bzg]

Hi Tassilo,

I don't have enough time at hand right now -- if you can come up with a patch for this, I'll gladly accept it!

Thanks in advance.

[stig]

I'm having indentation issues on things like this: https://github.com/Datomic/day-of-datomic/blob/master/src/datomic/samples/schema.clj#L50

(Just to add another datapoint.)

[sandhu]

The #289 PR that got merged this weekend should address this issue.

[tsdh]

Great, thanks!

[stig]

Hurrah!

[raxod502]

But it still looks like electric-indent-mode is broken? I.e. if I press RET in a Clojure docstring, then point is at column 0 until I press TAB at which point it moves to column 2.

[bzg]

The behavior I see in a docstring with clojure-mode version 5.7.0-snapshot is this one:

• RET goes at the beginning of the next line, independantly of electric-indent-mode
• C-j indents correctly with electric-indent-mode turned off, but do the same than RET otherwise

The docstring of C-j (aka electric-newline-and-maybe-indent) says:

Insert a newline. If ‘electric-indent-mode’ is enabled, that’s that, but if it is disabled then additionally indent according to major mode.

So I'm inclined to think RET and C-j are behaving correctly here, although I too think this is all a bit confusing. I've no good idea right on how to fix this right now.

Also beware that clojure-mode does not recognize docstring after the function's parameters:

(defn a-function
  "A well-placed docstring."
  []
  nil)

(defn another-function
  []
  "A misplaced docstring."
  nil)

[raxod502]

But the docstring for command newline (bound to RET) says this:

If ‘electric-indent-mode’ is enabled, this indents the final new line
that it adds, and reindents the preceding line.  To just insert
a newline, use M-x electric-indent-just-newline.

So your first bullet point sounds like wrong behavior of clojure-mode to me, since if electric-indent-mode is enabled then RET should indent the new line .

[bzg]

@raxod502 yes, you're right.

https://github.com/clojure-emacs/clojure-mode/pull/447 is an attempt at fixing the issue at point, please see the PR comment and let me know if this works for you as it works for me.

Thanks!