Skip to content
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

Refactor cider-docstring.el #3707

Closed
wants to merge 3 commits into from

Conversation

katomuso
Copy link
Contributor

@katomuso katomuso commented Jun 8, 2024

While I was trying to clean up cider-doc.el, I came across cider-docstring.el and noticed the cider-docstring--dumb-trim function. I don't know why it's named that way, but it looks pretty smart to me. Maybe too smart, as it does too many things, one of which, along with trimming, is formatting. So I think it is a good idea to separate it into two functions: cider-docstring--trim and cider-docstring--format.

The main reason for this is that these two functionalities are needed in different contexts. For example, we need to format the docstring when displaying it in the Doc or ClojureDocs buffer (notice that we don't need trimming here because it's a buffer and there's plenty of space to display the docstring), and we need trimming when displaying the docstring in the minibuffer or in a popup along with completions.

Also, the current formatting of the docstring in cider-docstring--dumb-trim is too opinionated. For example, it moves every single paragraph (signified by ". ") to a new line and adds an empty line after it, which totally messes up the original formatting (there are still newlines just in the middle of paragraphs, so we need to either remove all newlines and reformat the docstring from scratch, or intervene as little as possible). I guess nobody had noticed this strange formatting because not many people look for docstrings in the minibuffer or in completions, and even fewer care how it is formatted (but I do). Also, it removes a variable number of spaces from the beginning of every line. As we discussed in #3701, it will be safer to just remove 2 spaces from the beginning while performing formatting. Of course, later we can change this, and it will be much easier to do as we will need to change it in just one place (cider-docstring--format).

Additionally, there are a few small tweaks following this change:

  • Currently, when compact is non-nil when the Doc buffer rendering function is called, the docstring is passed to cider-docstring--dumb-trim before being displayed. As I explained earlier, it does two things: formatting and trimming. I think it will be more appropriate to always format the docstring and trim it only when a compact display is needed.
  • Replace the code that currently formats the docstring in the ClojureDocs buffer with a cider-docstring--format function call.
  • Use "…" character instead of "..." if trimming of the docstring is necessary, as it is a typographic character created just for that occasion and it takes up less space.

@katomuso katomuso changed the title Refactor cider-clojuredocs.el Refactor cider-docstring.el Jun 8, 2024
Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please optimize the PR for being understood (now and in a future)

  • Remove behavioral changes for now
    • These weren't introduced too long ago - simply changing them isn't the best way to start a discussion
  • Avoid commits in the form Refactor cider-clojuredocs.el
    • You're not saying what was done or why in the commit itself

I do appreciate the extended PR description, but for the long term it's better to place as much info as possible in the commits themselves.

An alternative being, create smaller PRs.

Thanks!


Also performs some bare-bones formatting, cleaning up some common whitespace issues."
(when s
(let* ((s (replace-regexp-in-string "\\. " ".\n\n" s)) ;; improve the formatting of e.g. clojure.core/reduce
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please undo the behavorial change. While we can eventually change it, I'd suggest that we work in a most collaborative fashion:

  • Create an issue with examples when you find something that seems problematic
    • Seek agreement for potentially controversial changes, and only then create a PR
  • PRs that conflate harmless changes with breaking changes are hard to review

(cider-docstring--dumb-trim fetched-doc)
fetched-doc)
(cider-docstring--format
(if compact (cider-docstring--trim fetched-doc) fetched-doc))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original if newlines are better since that way indentation can inform you of what belongs to 'then', and what belongs to 'else' (there can be multi-line elses in Elisp)

@katomuso
Copy link
Contributor Author

katomuso commented Jun 8, 2024

@vemv Thanks for the suggestions! I think you are right; there is too much going on, so I will create smaller PRs to be more gradual.

@katomuso katomuso closed this Jun 8, 2024
@katomuso katomuso deleted the docstring-refactoring branch June 8, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants