Nim: Some bugs with nimpretty

[ghost]

# comment

var x = 1

becomes

# comment


var x = 1

[ghost]

var x = 1
# comment

becomes

var x = 1                     # comment

[ghost]

proc a() =
  while true:
    discard
    # comment 1

  # comment 2
  discard

becomes

proc a() =
  while true:
    discard
  # comment 1

  # comment 2
  discard

[ghost]

(not false)

becomes

( not false)

[ghost]

Unhandled exception when file does not exist

λ nimpretty a
nimpretty.nim(75)        nimpretty
nimpretty.nim(71)        main
os.nim(528)              copyFile
oserr.nim(66)            raiseOSError
Error: unhandled exception: The system cannot find the file specified.
 [OSError]

Needs a simple try except here

[ghost]

if true: "true" else: "false"

becomes -

if true: "true"else: "false"

[ghost]

let a =
    if true: 15 # comment 1
    else: 27 # comment 2

becomes -

let a =
    if true: 15               # comment 1
    else: 27                  # comment 2

But

let a =
    if true: 15123123123.001230123 # comment 1
    else: 27 # comment 2

becomes

let a =
    if true: 15123123123.001230123 # comment 1
    else: 27                  # comment 2

i.e. comments are not aligned.

[ghost]

Bikeshedding -

proc longfunctionname(f1: proc (x: int, y: int, z: bool), f2: proc(x: bool, y: bool, z: int)) =
  discard

becomes -

proc longfunctionname(f1: proc (x: int; y: int; z: bool); f2: proc(x: bool;
    y: bool; z: int)) =
  discard

IMO it should become

proc longfunctionname(f1: proc (x: int; y: int; z: bool); 
    f2: proc(x: bool; y: bool; z: int)) =
  discard

This seems more readable to me.
(Here emphasis should be given on the fact that the 2nd parameter to long function name is split into the new line. Indentation should be whatever seems right)

Same when function call is made.

Same for function return type (Right now - it splits up the tuple fields into different line, IMO it would be better to have the whole return type on the next line, than half of its fields)

[ghost]

import strutils

echo "hello".toLowerAscii().toUpperAscii().toLowerAscii().toUpperAscii().toLowerAscii().toUpperAscii().toLowerAscii().toUpperAscii()

This gets split up as -

import strutils

echo "hello".toLowerAscii().toUpperAscii().toLowerAscii().toUpperAscii(
    ).toLowerAscii().toUpperAscii().toLowerAscii().toUpperAscii()

IMO, the closing bracket ) should have been on the same line, and the next line start with .

Also if the function takes arguments, then again splitting the arguments into next line is taking priority.
IMO even then the next line should start with a .functioncall() for readability purposes.

[ghost]

It does not take care of functions which have been already split by the developer

https://github.com/andreaferretti/patty/blob/fa58873be1cd729d9de3634a5897aacda3bcd890/patty.nim#L201-L204

becomes

newNimNode(nnkElifBranch).add(
      infix(newDotExpr(ident("a"), ident("kind")), "==", newDotExpr(ident("b"),
          ident("kind"))),
      condition
    ),

[dom96]

becomes -

proc longfunctionname(f1: proc (x: int; y: int; z: bool); f2: proc(x: bool;
   y: bool; z: int)) =
 discard

It should become:

proc longfunctionname(
  f1: proc (x: int, y: int, z: bool),
  f2: proc(x: bool, y: bool, z: int)
) =
  discard

I've recently become a big fan of this layout.

(btw, please make nimpretty output commas already).

[Araq]

It should become:

Meh, I think it's ugly and it is not in our style guide.

[dom96]

@Araq you need to be more specific. Also you should explain how you think it should look.

[andreaferretti]

The rule of thumb that I use is that if I cannot align things horizontally (because the line becomes too long), then I align them vertically, as @dom96 suggests

[Araq]

Also you should explain how you think it should look.

For me how nimpretty handles it is fine.

Nimpretty breaks up long lines by a simplistic scheme. If it gets it wrong, that means you can reformat this line on your own as long as it fits nimpretty's rules and nimpretty is designed to be cautious about the formatting the programmer did. If you don't like how nimpretty works, feel free to submit PRs but nimpretty's job is not to get arbitrary line wrapping "right". Maybe later versions will do that but so far it's out of its scope.

(btw, please make nimpretty output commas already).

I like semicolons more though and objectively it helps slightly in readability as there is a clearer distinction between "ident list" separation vs "type list" separation.

[dom96]

Semicolons in argument lists is a major showstopper for me to adopt nimpretty so please consider changing this.

[andreaferretti]

At the very minimum it should be configurable or respect the existing convention. Trying nimpretty on my own files generates giant diffs just because it changes every , into a ;

[dom96]

I'm not sure about making it configurable, AFAIK the idea behind a tool like this is to make all formatting consistent.

[andreaferretti]

Most tools that make formatting consistent accept a set of configurations, for instance scalafix

Since both , and ; are valid I do not see a way to enforce one or the other without making some users unhappy

[Araq]

I'm not sure about making it configurable, AFAIK the idea behind a tool like this is to make all formatting consistent.

Ok, how about this solution: The first usage in this context determines what nimpretty will use. If you use a ';', all places use ';' consistently, if you use ',', it doesn't touch it. (As it is sometimes required to be ';' for disambiguation.)

[andreaferretti]

@Araq I like this solution!

[timotheecour]

/cc @Araq @andreaferretti @SolitudeSF
actually how about the following simple rule instead, based on the following semantic difference bw , vs ; in param list:

proc foo(a, b: int; c, d: string) = discard # OK
proc foo(a, b: int, c, d: string) = discard # OK
proc foo(a; b: int, c, d: string) = discard # Error: typeless parameters are obsolete

rule

• use ; only after a list (length>1) of type-less arguments, right after the type, eg: a, b, c: int;
• use , everywhere else.

example

#input
proc foo(z: int, a, b: int, c, d: string, e: float; f: float)
#rewritten to:
proc foo(z: int, a, b: int; c, d: string; e: float, f: float)

in the above example, one , was transformed to ;, and one ; was transformed to , according to the suggested rule

[Araq]

I disagree that this rule is "simple". :-) I also see no benefits over what nimpretty currently does (I implemented the idea that I described above).

[andreaferretti]

@timotheecour The problem with your proposal is that it will generate giant diffs for some users ( some users prefer ,, others ;). The proposal by @Araq is conservative and will keep everyone happy with their prefered style