Change the way cider-docstring--format performs formatting
#3712
Conversation
katomuso
force-pushed
the
docstring-format-fix
branch
from
767e49b
to
698c0e4
Compare
cider-docstring.el
Outdated
| (split-string string "\n") | ||
| "\n"))) | ||
| string)) | ||
| ;; As this is a literal docstring from the source code and |
There was a problem hiding this comment.
You can probably mention in the docstring that this private function just trims leading whitespace or even remove it completely, as I now I don't see much need for it.
This change is user-facing, so it also needs a changelog entry. Might be a good idea to add some unit tests verifying how docstrings get displayed.
There was a problem hiding this comment.
We already use this function in a couple of places, and I think we should use it to format the docstring in the Doc buffer as well (so we can control indentation specifically in code for formatting Doc buffer because currently without proper formatting of the docstring, we have accidental indentation), so I strongly believe we should leave this function be.
There was a problem hiding this comment.
Regarding the docstring of the function, it will be unclear why we trim two spaces if we omit the specific detail that this is because the string is a literal string from the source code.
If you want, I can add the whole comment to the docstring, though there can be more rules, and it may not be that clear for which formatting rule this comment corresponds to.
There was a problem hiding this comment.
Fair enough, just keep it as is, then.
I find the terminology "literal docstring from the source code" to be a bit confusing (perhaps something like "raw Clojure docstring with its original formatting" would be better), but overall I don't have a great alternative proposal either. My remark for the description of the Elisp function was prompted mostly because it might be obvious to people why we need to reformat the Clojure docstrings at all.
There was a problem hiding this comment.
I see, that's a valid remark. Indeed, we should add some reasons to the docstring as to why we do formatting at all.
There was a problem hiding this comment.
I will also think about some unit tests.
There was a problem hiding this comment.
I've updated the docstring and also removed the comment as it felt redundant. I've also found a section in the community style guide related to docstring indentation (https://guide.clojure.style/#docstring-indentation). It might be worthwhile to include it in the docstring of the function.
Regarding unit tests, they don't make much sense for this one function, but they do make sense for the whole cider-docstring.el package. However, it first needs to be refactored, as it is a 10-year-old package and hard to make sense of.
Also, I was thinking about adding a changelog entry after another PR, in which we start using cider-docstring--format in the Doc buffer. Or should it be two separate entries?
There was a problem hiding this comment.
I've updated the docstring and also removed the comment as it felt redundant. I've also found a section in the community style guide related to docstring indentation (https://guide.clojure.style/#docstring-indentation). It might be worthwhile to include it in the docstring of the function.
Good idea.
Regarding unit tests, they don't make much sense for this one function, but they do make sense for the whole cider-docstring.el package. However, it first needs to be refactored, as it is a 10-year-old package and hard to make sense of.
Fine by me.
Also, I was thinking about adding a changelog entry after another PR, in which we start using cider-docstring--format in the Doc buffer. Or should it be two separate entries?
Sure, you can add this later.
There was a problem hiding this comment.
Okay, I've updated the docstring. Can we merge now?
katomuso
force-pushed
the
docstring-format-fix
branch
from
3cabf11
to
70a853f
Compare
katomuso
force-pushed
the
docstring-format-fix
branch
from
70a853f
to
ce772f5
Compare
Fix #3709