Skip to content

Allow single semicolon comments on a line #516

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

Closed
jeaye opened this issue Mar 17, 2019 · 21 comments
Closed

Allow single semicolon comments on a line #516

jeaye opened this issue Mar 17, 2019 · 21 comments

Comments

@jeaye
Copy link

jeaye commented Mar 17, 2019

Expected behavior

Single semicolon comments can be on their own line without messing up the indentation.

This was originally reported to SO here in 2010. I recognize that the recommended Lisp style is to have double semicolons for dedicated comment lines and single semicolons only for lines with code. However, even the core Clojure code and some of the most popular Clojure libraries have uses of single semicolons on dedicated lines (clojure.core, cljs.core, instaparse). So, I implore you to support this as a non-default option.

Furthermore, this is a pain point on my team at work, since some folks are in Emacs (using clojure-mode) and others are in Vim or IntelliJ where those semicolon indentation rules aren't a problem. The Clojure codebase was written before we had Emacs users on the team, so we have 15K+ lines of code using single semicolons and now the Emacs folks are having issues. We could change all of the code, but then everyone not using Emacs would need to change the way they write comments.

Concretely, applying indentation to a file containing this should not change anything (assuming this option is enabled):

; See below.
(defn foo []
  ; Wow, cool!
  (println "cool"))

Actual behavior

There is no configuration for this, so it's not currently possible to support. Lines containing single semicolon comments get indented by huge amounts when applying indentation to the line, range, form, or file.

Following the concrete example, the file becomes:

                                        ; See below.
(defn foo []
                                        ; Wow, cool!
  (println "cool"))

Steps to reproduce the problem

Create a new file in Clojure mode, add a single semicolon comment, like:

; uh oh!

and then apply indentation to the line or file (by pressing Tab or Enter, for example). The line will be indented a huge amount, rather than left exactly as it was. This also applies to evil mode (hitting ==, for example).

Environment & Version information

clojure-mode version information

This returned nil for me. I'm using Spacemacs, which may be interfering here. I'm a Vim user, primarily, but trying to do my best to repro.

clojure-mode (version nil)

Emacs version

26.1

Operating system

This has been reproduced on:

  • Arch Linux
  • Ubuntu Linux
  • macOS
@bbatsov
Copy link
Member

bbatsov commented Mar 20, 2019

I'm aware of the problem, but unfortunately this requires significant changes that I don't have time to do myself right now.

The core of the problem is that clojure-mode uses a lot of code straight from lisp-mode and that's code is kind of hard to change, without us just duplicating it. If it wasn't for this I would have added this config option long ago.

I'd gladly merge a PR if someone decides to tackle this.

@bbatsov
Copy link
Member

bbatsov commented Mar 20, 2019

Just noticed I created a task for me regarding this over 4 years ago. (#270) Long standing problem indeed.

@yuhan0
Copy link
Contributor

yuhan0 commented Mar 22, 2019

Doesn't seem to be that big of a change - here's the relevant code adapted from lisp-indent-line minus special handling for comments and spliced in place into clojure-indent-line

(defun clojure-indent-line ()
  "Indent current line as Clojure code."
  (interactive)
  (if (clojure-in-docstring-p)
      (save-excursion
        (beginning-of-line)
        (when (and (looking-at "^\\s-*")
                   (<= (string-width (match-string-no-properties 0))
                       (string-width (clojure-docstring-fill-prefix))))
          (replace-match (clojure-docstring-fill-prefix))))

    ;; `lisp-indent-line', but without special handling of comments
    (let ((pos (- (point-max) (point)))
          (indent (progn (beginning-of-line)
                         (calculate-lisp-indent (lisp-ppss)))))
      (when indent (indent-line-to indent))
      ;; If initial point was within line's indentation,
      ;; position after the indentation.  Else stay at same point in text.
      (if (> (- (point-max) pos) (point))
	        (goto-char (- (point-max) pos))))))

You might also want to change the value of comment-indent-function and comment-add in clojure buffers:

(add-hook 'clojure-mode-hook
          (lambda ()
            (setq comment-indent-function 'comment-indent-default)
            (setq comment-add 0)
            (comment-normalize-vars)))

I could open a PR for the above but it's not clear what the defaults should be, or if there should be some sort of configuration toggle?
It does seem like the 1-semicolon style is pretty common in the wild, and in clojure core libs.

@bbatsov
Copy link
Member

bbatsov commented Mar 22, 2019

Well, the default should be what it has always been, as it's generally a bad idea to change something that has been the default since day 1.

Generally we need some toggle - "clojure-semantic-comments" that's true by default.

Doesn't seem to be that big of a change - here's the relevant code adapted from lisp-indent-line minus special handling for comments and spliced in place into clojure-indent-line

But I think your change will break other commands relying on the current behavior - e.g. the various insert comment at point commands that Emacs has. Anyways, I'm glad you've decided to tackle this, but you should be careful as the behavior from lisp-mode trickles down to many places and we don't want to break something big.

                     (calculate-lisp-indent (lisp-ppss)))))

I'd prefer it if we copied those functions as well and tweaked them for Clojure if needed, so we can achieve true separation. I'm not sure how much code it's reasonable to share with lisp-mode, my preference would be as little as possible, but I don't have time to dwell on this much.

@yuhan0
Copy link
Contributor

yuhan0 commented Mar 22, 2019

Thanks for the reply - what I'll do (and welcome others to) is use this change in my local config for the next couple of days or weeks and see if it breaks anything else.
Do you have any hunch on what sort of places this change might trickle down to? It seemed pretty self-contained but I'm not at all familiar with all the intricacies of lisp-mode.

                 (calculate-lisp-indent (lisp-ppss)))))

I had a peek at that function but it's a huge 150-line body with other nested calls and what looks like plenty of mutable state to untangle - probably the sort of thing that's best left untouched :p

@bbatsov
Copy link
Member

bbatsov commented Mar 23, 2019

Do you have any hunch on what sort of places this change might trickle down to? It seemed pretty self-contained but I'm not at all familiar with all the intricacies of lisp-mode.

You and me both. :D Nothing comes to mind expect commands like M-;, but I guess those should be easy to get right.

Let's go with your idea and see how well it will work.

@bbatsov
Copy link
Member

bbatsov commented Mar 31, 2019

@yuhan0 Any progress with this?

@yuhan0
Copy link
Contributor

yuhan0 commented Apr 1, 2019

Hi, thanks for the follow up - I've been using the above change for more than a week now and noticed nothing out of the ordinary :)

(It actually helped fix a bug in my personal config, where I had bound indent-sexp to a custom key, not knowing that it was a lisp-mode specific command - the standard binding C-M-q is bound to prog-indent-sexp in clojure-mode which behaves as expected.)

@Frozenlock
Copy link

I tried clojure-indent-line as provided by @yuhan0 but it breaks in my Emacs once I press tab to indent:

calculate-lisp-indent: Symbol’s function definition is void: lisp-ppss

@yuhan0
Copy link
Contributor

yuhan0 commented Apr 17, 2019

Hmm, turns out that was something added relatively recently in Emacs 26:
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=4713dd425beac5cb459704e67dcb8f6faf714375

I assume you're using an older version of Emacs, It should work if you simply remove the (lisp-ppss) form and use the 0-argument call to calculate-lisp-indent.

@bbatsov
Copy link
Member

bbatsov commented Apr 17, 2019

Small update - I've decided to tackle this myself, so expect some fix in the next couple of days. I'm mostly wondering how deep down the rabbit hole do I want to go (how much code to inline in clojure-mode).

@Frozenlock
Copy link

Frozenlock commented May 8, 2019

Any news on this? If the problem is harder than expected, I could move to Emacs 26 and try the patch mentioned above.

@bbatsov
Copy link
Member

bbatsov commented May 8, 2019

I've been super busy lately and I dropped the ball on this. It's not super complex, but I need to find some time for it.

@jeaye
Copy link
Author

jeaye commented May 8, 2019

Another report: one of my teammates has been using @yuhan0's approach with no issue. Naturally, he's on a newer Emacs, but it seems to work well for those who are!

@Frozenlock
Copy link

Upgraded to Emacs 26.2 and it does indeed work.
I'll use this until it can be officially toggled in clojure-mode.
Thank you very much!

@j-cr
Copy link
Contributor

j-cr commented Dec 27, 2019

By the way, this can be fixed without modifying any code, just call this in your clojure-mode hook:

(setq-local comment-column 0)

clojure-mode-variables sets it to 40 by default. We can make the value customizable or just add it to the docs.

@svantevonerichsen6906
Copy link

I believe that setting comment-column to 0 is the right solution for a legacy codebase that one doesn't want to change, instead of codifying nonstandard comment styles.

@bbatsov
Copy link
Member

bbatsov commented Apr 19, 2020

@yuhan0 Btw, can you look into this one as well? You've been on a roll lately and it'd be nice to include this in the next release. I might eventually find time to revisit this myself, but for the next couple of weeks my focus is mostly nREPL and cider-nrepl.

@yuhan0
Copy link
Contributor

yuhan0 commented Apr 19, 2020

I actually found that @j-cr 's above solution of setting comment-column was the simplest way around the issue, and works on a project-specific basis with dir-local variables. So it seems right to just add a note about this to the docs instead of tampering with lisp-mode indentation code and introducing more configuration vars.

@bbatsov
Copy link
Member

bbatsov commented Apr 19, 2020

That's fine by me. I was struggling to decide whether it's best to do this or to continue decoupling things from lisp-mode.

@qleguennec
Copy link

I'm having an issue with this as well, because (setq-local comment-column 0) actually still messes with the indentation in the case where the comment is already indented. It indents it to 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants