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

Some bugs with nimpretty #8078

Closed
ghost opened this issue Jun 20, 2018 · 23 comments
Closed

Some bugs with nimpretty #8078

ghost opened this issue Jun 20, 2018 · 23 comments

Comments

@ghost
Copy link

@ghost ghost commented Jun 20, 2018

# comment

var x = 1

becomes

# comment


var x = 1
@ghost
Copy link
Author

@ghost ghost commented Jun 20, 2018

var x = 1
# comment

becomes

var x = 1                     # comment

@ghost
Copy link
Author

@ghost ghost commented Jun 20, 2018

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

  # comment 2
  discard

becomes

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

  # comment 2
  discard

@ghost
Copy link
Author

@ghost ghost commented Jun 20, 2018

(not false)

becomes

( not false)

@ghost
Copy link
Author

@ghost ghost commented Jun 20, 2018

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
Copy link
Author

@ghost ghost commented Jun 20, 2018

if true: "true" else: "false"

becomes -

if true: "true"else: "false"

@ghost
Copy link
Author

@ghost ghost commented Jun 20, 2018

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
Copy link
Author

@ghost ghost commented Jun 20, 2018

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
Copy link
Author

@ghost ghost commented Jun 20, 2018

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
Copy link
Author

@ghost ghost commented Jun 20, 2018

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
    ),

Araq added a commit that referenced this issue Jun 20, 2018
@dom96
Copy link
Member

@dom96 dom96 commented Jun 22, 2018

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
Copy link
Member

@Araq Araq commented Jun 24, 2018

It should become:

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

@dom96
Copy link
Member

@dom96 dom96 commented Jun 24, 2018

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

@andreaferretti
Copy link
Collaborator

@andreaferretti andreaferretti commented Jun 25, 2018

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
Copy link
Member

@Araq Araq commented Jun 25, 2018

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
Copy link
Member

@dom96 dom96 commented Jun 25, 2018

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

@andreaferretti
Copy link
Collaborator

@andreaferretti andreaferretti commented Jun 25, 2018

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
Copy link
Member

@dom96 dom96 commented Jun 25, 2018

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

@andreaferretti
Copy link
Collaborator

@andreaferretti andreaferretti commented Jun 25, 2018

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
Copy link
Member

@Araq Araq commented Jun 25, 2018

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
Copy link
Collaborator

@andreaferretti andreaferretti commented Jun 26, 2018

@Araq I like this solution!

@timotheecour
Copy link
Member

@timotheecour timotheecour commented Jul 13, 2018

/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
Copy link
Member

@Araq Araq commented Jul 13, 2018

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
Copy link
Collaborator

@andreaferretti andreaferretti commented Jul 16, 2018

@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

narimiran added a commit to narimiran/Nim that referenced this issue Nov 1, 2018
narimiran added a commit that referenced this issue Nov 1, 2018
narimiran added a commit that referenced this issue Nov 1, 2018
(cherry picked from commit 3ee53a7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants