ALSchwalm / janet-mode

A major mode for the janet programming language: https://janet-lang.org/
GNU General Public License v3.0
46 stars 14 forks source link

janet-mode indents source files improperly. #8

Open crocket opened 3 years ago

crocket commented 3 years ago

Especially, with and let.

[] breaks indentation.

sogaiu commented 3 years ago

I had some difficulty reproducing this, but I think with the following code, it may be possible:

(defn- dotenv [key]
  (when (os/stat ".env")
    (with [f (file/open ".env")]
      (-> (file/read f :all)
          (parse-dotenv)
          (get key)))))

With only that in a buffer with janet-mode enabled, pressing Enter when the cursor is on the closing square bracket of the with binding, I get:

(defn- dotenv [key]
  (when (os/stat ".env")
    (with [f (file/open ".env")
          ]
      (-> (file/read f :all)
          (parse-dotenv)
          (get key)))))

The closing square bracket seems to be off by one to the left.

FWIW, typing a single character yields:

(defn- dotenv [key]
  (when (os/stat ".env")
    (with [f (file/open ".env")
          a]
      (-> (file/read f :all)
          (parse-dotenv)
          (get key)))))

Subsequently, pressing Tab does adjust the indentation correctly:

(defn- dotenv [key]
  (when (os/stat ".env")
    (with [f (file/open ".env")
           a]
      (-> (file/read f :all)
          (parse-dotenv)
          (get key)))))

For reference the source is from: https://github.com/joy-framework/dotenv/blob/5e0ce9b9bd5db653400b7d401cbc2329ab47cbbd/dotenv.janet#L12-L17

crocket commented 3 years ago

This is what I get.

Either

(defn- dotenv [key]
  (when (os/stat ".env")
    (with [f (file/open ".env")]
          (-> (file/read f :all)
              (parse-dotenv)
              (get key)))))

or

(defn- dotenv [key]
  (when (os/stat ".env")
    (with
     [f (file/open ".env")]
     (-> (file/read f :all)
         (parse-dotenv)
         (get key)))))
sogaiu commented 3 years ago

Ah I see. Yes, I can get that too.

Thanks for elaborating.

sogaiu commented 3 years ago

For comparison purposes, I ran the 2 results in https://github.com/ALSchwalm/janet-mode/issues/8#issuecomment-762838001 through jfmt ( b27dff6bb32b89b20462eec33f50c1583c301b0a ) and got:

(defn- dotenv [key]
  (when (os/stat ".env")
    (with [f (file/open ".env")]
      (-> (file/read f :all)
          (parse-dotenv)
          (get key)))))

and:

(defn- dotenv [key]
  (when (os/stat ".env")
    (with
      [f (file/open ".env")]
      (-> (file/read f :all)
          (parse-dotenv)
          (get key)))))
sogaiu commented 3 years ago

IIUC, jfmt is basically a wrapper aroung spork/fmt. Possibly a relevant section of code is:

      [:array xs] (emit-body "@[" xs "]")
      [:btuple xs] (emit-body "[" xs "]")
      [:ptuple xs] (if (check-indent-2 xs)
                     (emit-body "(" xs ")" 1)
                     (emit-funcall xs))
      [:struct xs] (emit-body "{" xs "}")
      [:table xs] (emit-body "@{" xs "}")

via: https://github.com/janet-lang/spork/blob/714c216ac7f7fb13ece2d9735fa608f1962874e2/spork/fmt.janet#L160-L166

IIUC, roughly this translates to formatting things that are not paren tuples but are "containers" (i.e. arrays, bracket tuples, structs, and tables).

Separately, paren tuples have 2 cases:

The former is chosen if check-indent-2 returns a truthy result:

(defn- check-indent-2
  "Check if a tuple needs a 2 space indent or not"
  [items]
  (if-let [[tag body] (get items 0)]
    (cond
      (= "\n" (get items 1)) true
      (not= tag :span) nil
      (in indent-2-forms body) true
      (peg/match indent-2-peg body) true)))

via: https://github.com/janet-lang/spork/blob/714c216ac7f7fb13ece2d9735fa608f1962874e2/spork/fmt.janet#L89-L97

That in turn depends on:

(def- indent-2-forms
  "A list of forms that are control forms and should be indented two spaces."
  (invert ["fn" "match" "with" "with-dyns" "def" "def-" "var" "var-" "defn" "defn-"
           "varfn" "defmacro" "defmacro-" "defer" "edefer" "loop" "seq" "generate" "coro"
           "for" "each" "eachp" "eachk" "case" "cond" "do" "defglobal" "varglobal"
           "if" "when" "when-let" "when-with" "while" "with-syms" "with-vars"
           "if-let" "if-not" "if-with" "let" "short-fn" "try" "unless" "default" "forever"
           "repeat" "eachy" "forv"]))

(def- indent-2-peg
  "Peg to use to fuzzy match certain forms."
  (peg/compile ~(+ "with-" "def" "if-" "when-")))

via: https://github.com/janet-lang/spork/blob/714c216ac7f7fb13ece2d9735fa608f1962874e2/spork/fmt.janet#L76-L87

crocket commented 3 years ago

I don't understand. Have you found a solution already?

sogaiu commented 3 years ago

In short, no.

The purpose of my previous comment was to start making it clear how jfmt / spork/fmt works. AFAIU, that is code that bakpakin wrote and I presume it's worth trying to emulate those indentation "rules". However, in order to do so, I think it will help to understand what they consist of -- hence the previous comment with some details.

I did not find the janet-mode indentation code to be particularly clear. I have studied it, racket-mode, and clojure-mode a bit (the latter two which influenced janet-mode's indenting) and AFAICT, none of these three modes appear to have a straight-forward description of how their indentation works.

I'm still looking into both spork/fmt and janet-mode though.

sogaiu commented 3 years ago

@crocket I tried changing:

              ((string-match (rx bos (or "def" "with-")) head)

to:

              ((string-match (rx bos (or "def" "with")) head)

It's a removal of "-" on the line at: https://github.com/ALSchwalm/janet-mode/blob/master/janet-mode.el#L271

Does with work any better for you that way?

crocket commented 3 years ago

After changing

((string-match (rx bos (or "def" "with-")) head)

to

((string-match (rx bos (or "def" "with")) head)

I get

(defn- dotenv [key]
  (when (os/stat ".env")
    (with [f (file/open ".env")]
      (-> (file/read f :all)
          (parse-dotenv)
          (get key)))))

What did you do?

sogaiu commented 3 years ago

@crocket Thanks for trying the change.

IIUC, this code:

              ((string-match (rx bos (or "def" "with-")) head)
               body-indent) ;just like 'defun

affects indentation of certain tuples.

Such tuples begin with a symbol that starts with either the string "def" or the string "with-".

Before the change, with-dyns would be indented one way, but with would not be because the regular expression described by:

(rx bos (or "def" "with-"))

matches with-dyns, but not with.

With the change, with-dyns and with are handled in the same fashion.

So would you say that at least for with the indentation results are more in line with what you expect?

crocket commented 3 years ago

Yes.

sogaiu commented 3 years ago

@crocket Thanks.

I did not figure out a case where let did not get indented properly.

Do you have a case that demonstrates the issue? If so, would you mind sharing it?

crocket commented 3 years ago
(let
    [var1 (var1)
     var2 (var2)]
  (call something))

should look like

(let
  [var1 (var1)
   var2 (var2)]
  (call something))
sogaiu commented 3 years ago

@crocket Thanks for the examples. I will investigate.

sogaiu commented 3 years ago

@crocket

If you comment out: https://github.com/ALSchwalm/janet-mode/blob/2f5bcabcb6953e1ed1926ba6a2328c453e8b4ac7/janet-mode.el#L386

and change: https://github.com/ALSchwalm/janet-mode/blob/2f5bcabcb6953e1ed1926ba6a2328c453e8b4ac7/janet-mode.el#L271

to:

              ((string-match (rx bos (or "def" "with" "let")) head)

Does it work as you expect?

crocket commented 3 years ago

Yes. What did you do with (let 1)?

sogaiu commented 3 years ago

@crocket Thanks for reporting back.

In the description below, please keep the following in mind: https://github.com/ALSchwalm/janet-mode/blob/2f5bcabcb6953e1ed1926ba6a2328c453e8b4ac7/janet-mode.el#L265-L272

Which path is ultimately followed affects the resulting indentation.

Commenting out (let 1), causes the (intergerp method) part of: https://github.com/ALSchwalm/janet-mode/blob/2f5bcabcb6953e1ed1926ba6a2328c453e8b4ac7/janet-mode.el#L265-L266 to fail. Assuming the other change is also made, the following eventually gets executed:

              ((string-match (rx bos (or "def" "with" "let")) head)
               body-indent) ;just like 'defun

An alternative approach is to only change: https://github.com/ALSchwalm/janet-mode/blob/2f5bcabcb6953e1ed1926ba6a2328c453e8b4ac7/janet-mode.el#L386

to:

         (let defun)

That should lead to: https://github.com/ALSchwalm/janet-mode/blob/2f5bcabcb6953e1ed1926ba6a2328c453e8b4ac7/janet-mode.el#L267-L268 being the executed code.

The key is that either: https://github.com/ALSchwalm/janet-mode/blob/2f5bcabcb6953e1ed1926ba6a2328c453e8b4ac7/janet-mode.el#L268 or: https://github.com/ALSchwalm/janet-mode/blob/2f5bcabcb6953e1ed1926ba6a2328c453e8b4ac7/janet-mode.el#L272 being executed appears to produce the desired indentation.

At the moment it seems to me that the alternate approach (i.e. using (let defun)) is better. The other approach (the one explained in an earlier comment) will also match other symbols that begin with let. It's not clear to me whether that's a good diea.