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

Auto-indent according to guidelines #3

Closed
PEZ opened this issue Mar 31, 2018 · 21 comments
Closed

Auto-indent according to guidelines #3

PEZ opened this issue Mar 31, 2018 · 21 comments
Assignees

Comments

@PEZ
Copy link
Collaborator

PEZ commented Mar 31, 2018

In issue #2, @tjbrandt writes:

For formatting/indenting, a good example is using a let form.

Consider the following snippet:

(let [x :first-val
      y :second-val]
  (println x y))

As you're typing that out, hitting the return key after :first-val should, if honoring the style guide, put your cursor directly below x, so that when you then type y, the x and y should be vertically aligned. However, VSCode instead either puts you back at the beginning of the next line (if auto-indent is off) or indents you only 2 spaces (if auto-indent is on).

But then, hitting return after typing the closing bracket ] after :second-val, the cursor should then be placed on the next line and indented two spaces, according to the style guide. But VSCode "intelligently" decides that you're trying to indent everything at the same indentation as the last couple variables, so it then puts you directly under the y, even though this time that's not what you wanted.

@PEZ PEZ self-assigned this Mar 31, 2018
@PEZ
Copy link
Collaborator Author

PEZ commented Mar 31, 2018

I'm looking at this now a bit. Starting with #4, though because that would make this issue a bit less painful.

@PEZ
Copy link
Collaborator Author

PEZ commented Mar 31, 2018

OK, #4 is fixed, so now the documents should be formatted on save if you have that setting enabled. While it would be much better to have the code auto indented as you type, this might make it less painful. I'll keep looking at this issue and see if I can figure out a good way to do it.

@trevor-brandt
Copy link
Contributor

Cool! You mentioned I might want to add more examples, so I'll provide some more here.

Also, though, www.4clojure.com has a great auto-identer for their text fields. Just go there, pick a problem from the problem list, and start typing some clojure into the text box. You'll see what I mean.

Here are some examples.

If an argument list is broken up between lines, the first argument on the second line should vertically align with the first argument on the first line:

(my-func arg1
         arg2
         arg3)

(my-func first-arg second-arg
         another-arg a-fourth-arg)

Same thing with any collections (maps, vectors, etc), although since they don't have a function in the form, all items should be considered as if they were arguments:

{:this thing
 :another thang}

[first-item second-item
 another-item fourth-item]

Similar to let, macros/special-forms that have body parameters should have their body parameters indented two spaces (e.g., defn, when, do, if):

(defn my-function
  [arg1 arg2]
  (first-body-form)
  (next-body-form with-arg
                  with-another-arg))

(if some-condition?
  (do-this)
  (do-that))

(let [my-var (if some-condition?
               "hi!"
               (something-else))]
  (process my-var))

(when (some-test-fn some-var)
  (do-something arg1))

(do
  (first-thing)
  (second-thing with-arg))

Multi-arity functions also have the body forms vertically aligned with their argument vector, similar to single-arity, but it looks a little unorthodox at first because there's a single-space indentation there (line 3 and 5):

(defn multi-arity-fn
  ([arg1]
   (multi-arity-fn arg1 nil))
  ([arg1 arg2]
   (do-some-stuff arg1 arg2)
   (do-some-more-stuff)))

PEZ pushed a commit that referenced this issue Apr 2, 2018
@PEZ
Copy link
Collaborator Author

PEZ commented Apr 2, 2018

Wow, many thanks for this! Before I realised you had provided this I pushed a change that should fix many of the cases, I think. Please try it out and report back if you agree. I'll close the issue soon and we can open separate issues for any remaining cases.

@trevor-brandt
Copy link
Contributor

You're fast! On first pass, was working great, but I think it's interacting a little unpredictably with the parinfer extension as I'm using it a little more. Can you tell me whether you're testing with parinfer, and if so, whether you're in "indent" mode or "paren" mode?

I think your auto-indentation should have a config flag so I can turn it off, just in case someone doesn't like it or it interacts unpredictably with another plugin.

I think the optimal combo would be to have your plugin handle indentation and parinfer handle parens, which means you'd be using indent mode:

Indent Mode gives you full control of indentation, while Parinfer corrects parens.

In this case, Indent Mode would give Calva (new name! :) ) full control of indentation, while Parinfer corrects parens.

For now, I'll turn off Parinfer and let you know if I see anything that's not formatting right with your auto-indentation.

@trevor-brandt
Copy link
Contributor

Disabled Parinfer, and found it working really well. There are only a couple things so far that need correcting:

The multi-arity function behaves a little strangely. It seems the first body form in a multi-arity function tries to vertically align with the last arg in the arg vector:

(defn multi-arity
  "This is a multi-arity fn"
  ([test] (multi-arity test nil))
  ([test arg]
         (indented-to-here)
   (but-should-have-indented-to-here)))

But this works correctly, which is awesome:

(some-func [arg1 arg2]
           arg3
           arg4)

So I think the multi-arity thing is specific to having a vector as the first thing in a list form, which is really specific to multi-arity functions I think.

The other thing I noticed turns out to really be an edge case, but still worth fixing. When typing out a map form starting in the very first column of a file, when you hit enter after the first line, it puts a space in front of the cursor instead of behind the cursor, which results in this (notice the space after thang):

{:this thing
:that thang }

But I was pleasantly surprised to find that these all work great:

(let [some-var {:this thing
                :that thang}]
  (that-map-worked-great))

(some-func {:this map
            :also works
            :perfect ly})

Nice work so far! I think the odd interaction with Parinfer is basically Parinfer's fault. I noticed the github page for the vscode version now says the developer will be getting back to it later in the year, which would be awesome. Works really well in Atom, and I know he wants feature- and quality-parity between the two, so I'll be looking forward to that as well.

@trevor-brandt
Copy link
Contributor

Quick follow-up: for the second issue I mentioned (the issue with the map), the same thing happens with vectors and sets. It's probably all the same code handling it in the extension, so fixing one will probably fix all, but thought it's worth giving a couple more test cases:

#{item1
item2
item3    }

[item1
item2
item3  ]

There is another issue I found. I have three different examples, but I think they're all the same thing. Basically, when you are calling a function, if you're approaching your 80 character limit on the line and want to put the first argument on a new line, the function name and the argument should be vertically aligned (i.e., the argument should be indented in just that one space):

(if-this-function-name-approches-the-80-char-line
 put-the-arg-here
 and-the-next-arg-here)

The indentation is actually doing this (and behavior is similarly incorrect for anonymous functions and lists):

(some-function
               arg-idents-to-here-when-you-hit-enter
               second-arg)

#(some-anonymous-function
                          some-arg
                          second-arg)

(map #(some-really-long-func-name
                                  %
                                  arg2)
     [item1 item2])

'(list-item1
             list-item2
             list-item3)

@trevor-brandt
Copy link
Contributor

Another one. loop should behave like let as far as the bindings are concerned. There are probably other similar macros out there that should behave the same way, but I can't think of them right now.

Actual behavior:

(loop [first-binding :value
  second-binding :other-value]
  (loop-execution-here))

Desired behavior:

(loop [first-binding :value
       second-binding :other-value]
  (loop-execution-here))

@trevor-brandt
Copy link
Contributor

Another one: when a form is complete (the final matching paren is typed) and you hit enter, your indentation level should go back to where the form started.

Actual behavior:

(defn test-func
  [arg1]
  (do-something arg1))

  (defn new-function-is-indented
    []
    (do-something-else))

Desired behavior:

(defn test-func
  [arg1]
  (do-something arg1))

(defn new-function-should-not-be-indented
  []
  (do-something-else))

PEZ pushed a commit that referenced this issue Apr 2, 2018
@PEZ
Copy link
Collaborator Author

PEZ commented Apr 2, 2018

This is great feedback!

Though, it does seem like my extension is clashing with Parinfer now. 😢 When I worked with the auto-indentation I disabled everything that I thought could interfere and forgot to enable Parinfer when my indentation seemed to work.

There might be other things differing between yours and mine setup, because for me the (loop form works as well as the completed case. What OS and such are you using?

Released a new version with a sort of hackish way to deal with maps, vectors and sets not being treated well. Please check it out. ❤️

@PEZ
Copy link
Collaborator Author

PEZ commented Apr 2, 2018

There is the case when parens and brackets are used inside strings and comments, that they will still be counted for indentation. I think I know how to avoid that, but it is not implemented yet, FYI.

@trevor-brandt
Copy link
Contributor

You're right that loop was working even before I updated. Maybe it was still strange carry-over functionality from parinfer.

Speaking of parinfer.... is there any way for your plugin to force another plugin's code execution to happen after yours? I think if you could guarantee that parinfer will always execute in the editor after calva, then calva would handle the auto-indenting and parinfer can adjust parens accordingly. I don't know if that's possible though.

As for specific examples, these are looking good:

(loop [one :binding
       another :binding]
  (looking-good!))

{:this thing
 :another thing}

#{item1
  item2
  item3}

So I think the only outstanding ones are these, which I think are just multiple examples of a single issue:

'(test1
        test2
        test3)

(really-long-function-name
                           first-arg
                           second-arg)

#(really-long-function-name-for-anonymous-function
                                                   arg1
                                                   arg2)

@trevor-brandt
Copy link
Contributor

trevor-brandt commented Apr 2, 2018

Side note: I think the cmd-click to definitions might be broken, or at least it isn't working on my machine as of this morning. I hadn't used that feature over the weekend, but I do know it was working Friday during business hours. (I'm on Pacific Time in the US.)

I'm using OSX High Sierra 10.13.3, VSCode 1.12.1, most recent Calva extension, and the same versions of the two dependencies you suggest putting in profiles.clj:

{:user {:plugins [[cider/cider-nrepl "0.16.0"]]
        :dependencies [[org.clojure/tools.nrepl "0.2.12"]]}

I have these two calva settings set at the user level, otherwise all defaults:

    "clojure4vscode.autoConnect": false,
    "clojure4vscode.lintOnSave": true,

I open VSCode, open the integrated terminal, start a REPL with lein repl, and once it's up and running successfully, I connect by clicking the "click to connect" in the status bar. After that, it shows the correct repl port down there, and I can in-line evaluate code or send selections to the repl. However, the only functions I can cmd-click on to go to its source are clojure.core functions.

Is it still working on your machine?

EDIT: here is an exception and stack trace I'm getting in the REPL itself. I think it might be happening when it's trying to run the tests on save, and I don't know if it could be related to the cmd-click issues. Today is the first I've seen the exception, and I see it in more than one project:

user=> 2018-04-02 16:33:05.194 ERROR [clojure-agent-send-off-pool-9] clojure.tools.nrepl.server  - Unhandled REPL handler exception processing message {:ns user, :op test, :session 74baaa00-8e85-421e-9e7f-abac875e9d54}
clojure.lang.Compiler$CompilerException: java.lang.IllegalArgumentException:Bad binding form, expected symbol, got: [ns vars], compiling:(cider/nrepl/middleware/test.clj:191:3)
        at clojure.lang.Compiler.analyzeSeq(Compiler.java:7010)
        at clojure.lang.Compiler.analyze(Compiler.java:6773)
        at clojure.lang.Compiler.analyzeSeq(Compiler.java:6991)
        at clojure.lang.Compiler.analyze(Compiler.java:6773)
        at clojure.lang.Compiler.analyze(Compiler.java:6729)
        at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2822)
        at clojure.lang.Compiler.analyzeSeq(Compiler.java:7003)
        at clojure.lang.Compiler.analyze(Compiler.java:6773)
        at clojure.lang.Compiler.analyze(Compiler.java:6729)
        at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:6100)
        at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6420)
        at clojure.lang.Compiler.analyzeSeq(Compiler.java:7003)
        at clojure.lang.Compiler.analyze(Compiler.java:6773)
        at clojure.lang.Compiler.analyzeSeq(Compiler.java:6991)
        at clojure.lang.Compiler.analyze(Compiler.java:6773)
        at clojure.lang.Compiler.analyzeSeq(Compiler.java:6991)
        at clojure.lang.Compiler.analyze(Compiler.java:6773)
        at clojure.lang.Compiler.analyze(Compiler.java:6729)
        at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:6100)
        at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5460)
        at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4022)
        at clojure.lang.Compiler.analyzeSeq(Compiler.java:7001)
        at clojure.lang.Compiler.analyze(Compiler.java:6773)
        at clojure.lang.Compiler.analyzeSeq(Compiler.java:6991)
        at clojure.lang.Compiler.analyze(Compiler.java:6773)
        at clojure.lang.Compiler.access$300(Compiler.java:38)
        at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:595)
        at clojure.lang.Compiler.analyzeSeq(Compiler.java:7003)
        at clojure.lang.Compiler.analyze(Compiler.java:6773)
        at clojure.lang.Compiler.analyze(Compiler.java:6729)
        at clojure.lang.Compiler.eval(Compiler.java:7066)
        at clojure.lang.Compiler.load(Compiler.java:7514)
        at clojure.lang.RT.loadResourceScript(RT.java:379)
        at clojure.lang.RT.loadResourceScript(RT.java:370)
        at clojure.lang.RT.load(RT.java:460)
        at clojure.lang.RT.load(RT.java:426)
        at clojure.core$load$fn__6548.invoke(core.clj:6046)
        at clojure.core$load.invokeStatic(core.clj:6045)
        at clojure.core$load.doInvoke(core.clj:6029)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.core$load_one.invokeStatic(core.clj:5848)
        at clojure.core$load_one.invoke(core.clj:5843)
        at clojure.core$load_lib$fn__6493.invoke(core.clj:5888)
        at clojure.core$load_lib.invokeStatic(core.clj:5887)
        at clojure.core$load_lib.doInvoke(core.clj:5868)
        at clojure.lang.RestFn.applyTo(RestFn.java:142)
        at clojure.core$apply.invokeStatic(core.clj:659)
        at clojure.core$load_libs.invokeStatic(core.clj:5925)
        at clojure.core$load_libs.doInvoke(core.clj:5909)
        at clojure.lang.RestFn.applyTo(RestFn.java:137)
        at clojure.core$apply.invokeStatic(core.clj:659)
        at clojure.core$require.invokeStatic(core.clj:5947)
        at clojure.core$require.doInvoke(core.clj:5947)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at cider.nrepl$run_deferred_handler$fn__21646.invoke(nrepl.clj:37)
        at clojure.lang.Delay.deref(Delay.java:42)
        at clojure.core$deref.invokeStatic(core.clj:2312)
        at clojure.core$deref.invoke(core.clj:2298)
        at cider.nrepl$wrap_test$fn__21816.invoke(nrepl.clj:380)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at cider.nrepl$wrap_refresh$fn__21784.invoke(nrepl.clj:316)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at cider.nrepl$wrap_resource$fn__21792.invoke(nrepl.clj:342)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at cider.nrepl$wrap_stacktrace$fn__21808.invoke(nrepl.clj:370)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at cider.nrepl$wrap_trace$fn__21824.invoke(nrepl.clj:397)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at cider.nrepl$wrap_tracker$fn__21832.invoke(nrepl.clj:415)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at clojure.tools.nrepl.middleware.session$add_stdin$fn__19497.invoke(session.clj:238)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at cider.nrepl$wrap_apropos$fn__21692.invoke(nrepl.clj:129)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at cider.nrepl$wrap_complete$fn__21708.invoke(nrepl.clj:143)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at cider.nrepl$wrap_ns$fn__21768.invoke(nrepl.clj:279)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at cider.nrepl$wrap_debug$fn__21718.invoke(nrepl.clj:163)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at cider.nrepl.middleware.pprint$handle_pprint_fn.invokeStatic(pprint.clj:50)
        at cider.nrepl.middleware.pprint$handle_pprint_fn.invoke(pprint.clj:45)
        at clojure.lang.Var.invoke(Var.java:385)
        at cider.nrepl$wrap_pprint_fn$fn__21674.invoke(nrepl.clj:90)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at clojure.tools.nrepl.middleware.session$session$fn__19482.invoke(session.clj:192)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at cider.nrepl$wrap_spec$fn__21800.invoke(nrepl.clj:354)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at cider.nrepl$wrap_enlighten$fn__21726.invoke(nrepl.clj:189)
        at clojure.tools.nrepl.middleware$wrap_conj_descriptor$fn__18089.invoke(middleware.clj:22)
        at clojure.tools.nrepl.server$handle_STAR_.invokeStatic(server.clj:19)
        at clojure.tools.nrepl.server$handle_STAR_.invoke(server.clj:16)
        at clojure.tools.nrepl.server$handle$fn__19555.invoke(server.clj:28)
        at clojure.core$binding_conveyor_fn$fn__5476.invoke(core.clj:2022)
        at clojure.lang.AFn.call(AFn.java:18)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.IllegalArgumentException: Bad binding form, expected symbol, got: [ns vars]
        at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6363)
        at clojure.lang.Compiler.analyzeSeq(Compiler.java:7003)
        ... 99 common frames omitted

@PEZ
Copy link
Collaborator Author

PEZ commented Apr 3, 2018

That's bad!

It is still working on my machine, though… Can you try with the editor.formatOnType switched of for Clojure? (It's what is used for the auto indent adjustments Calva is doing.)

Also, the compiler seems to complain about a bad ns form, maybe the auto indent thing has messed something up todo with that for you…

@PEZ
Copy link
Collaborator Author

PEZ commented Apr 3, 2018

About outstanding issues (except for the worrying mystery with evaluation of files and running tests failing for you). We have:

  1. Parinfer clash (really don't know how to deal with this).
  2. Multi-arity function bodies getting the wrong treatment when on their own line.
  3. Arguments to ”non-special” forms getting the wrong treatment when the first argument is on a line of its own.

I'll make a keyboard shortcut for toggling the indent-correction so as to ease the coexistence with Parinfer, for now. And have look at 2 and 3, which shouldn't be too tricky to fix.

(2 actually gets better treatment with Parinfer enabled, 3 clashes totally with Parinfer.)

@PEZ
Copy link
Collaborator Author

PEZ commented Apr 3, 2018

New version realeased. Added a command with keyboard shortcut alt+v tab for toggling the indentation adjustment on and off.

@trevor-brandt
Copy link
Contributor

Downloaded the new version. It's working more consistently now. Toggling indentation adjustment doesn't seem to affect the "go to definition" functionality one way or another (but the toggle key shortcut does work well).

It seems like cmd-click works for most functions, but there is a namespace or two where cmd-click doesn't even work inter-project (i.e., one file won't allow you to go to a definition in another file). It could be something messy with the project, though, because I think it's a specific namespace or two.

I haven't seen the exception being thrown since I updated, so that's great.

So I think we're ok now! Sorry for the false alarm. Not sure what was happening before. ¯_(ツ)_/¯

@PEZ
Copy link
Collaborator Author

PEZ commented Apr 3, 2018

There are many moving parts, no wonder some false alarms are set off! 😄

I have just published a new version that I hope fixes multi-arity and folding of arguments below non-special forms when the first line does not contain arguments. (Phew, don't know if that made any sense, but the last known indentation quirks, anyway.) I might have introduced some new problems, of course. Please help me test it.

@trevor-brandt
Copy link
Contributor

Working great! Thanks!

@PEZ PEZ closed this as completed Apr 3, 2018
@PEZ
Copy link
Collaborator Author

PEZ commented Apr 4, 2018

Many thanks for the sparkling review! It is now keeping me warm here in a Stockholm where spring and winter are still fighting it out.

@trevor-brandt
Copy link
Contributor

Ha, you're welcome! Stay warm!

trevor-brandt pushed a commit to trevor-brandt/clojure4vscode that referenced this issue May 3, 2018
trevor-brandt pushed a commit to trevor-brandt/clojure4vscode that referenced this issue May 3, 2018
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

No branches or pull requests

2 participants