-
-
Notifications
You must be signed in to change notification settings - Fork 643
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.
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
Change the way cider-docstring--format
performs formatting
#3712
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will also think about some unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've updated the docstring. Can we merge now?
3cabf11
to
70a853f
Compare
70a853f
to
ce772f5
Compare
Fix #3709