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

nimpretty aligns comment annoyingly #9399

Closed
timotheecour opened this issue Oct 16, 2018 · 9 comments

Comments

@timotheecour
Copy link
Contributor

commented Oct 16, 2018

proc foobar(a: int, b: int, body: untyped) = discard

proc testFoobar() =
  let firstFar = 1
  let secondVar = 2
  foobar(firstFar, secondVar):
    const a = 1 # checks it's const

wrongly aligns comment as:

proc foobar(a: int, b: int, body: untyped) = discard

proc testFoobar() =
  let firstFar = 1
  let secondVar = 2
  foobar(firstFar, secondVar):
    const a = 1               # checks it's const
@Araq

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

That's actually a feature and works as intended but I can see why it's kinda annoying. :P

I have a better idea of how to produce these alignments...

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

That's actually a feature and works as intended

  • which one works as intended, case1, case2 or both?

  • one thing to keep in mind is minimizing diffs triggered by unrelated changes, eg:
    for case2, changing foobar(firstFar, secondVar): to foobar(firstFar, secondVar, thirdVar): should not change alignment of comment below

@Araq

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Both.

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

@Araq it seems alignment and comment reflowing issues are the main thing preventing widespread use of nimpretty ; In #8458 I suggested a nimpretty --safe option that would just run a "safe" subset of all what nimpretty does, so it could simply exclude alignment and comment reflowing until these alignment issues are ironed out; as it stands, we can't use nimpretty in the wild ; with nimpretty --safe we could

@Araq

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

Implementing nimpretty --safe is as much work as fixing nimpretty.

@krux02 krux02 changed the title [nimpretty] comment wrongly aligned nimpretty aligns comment annoyingly Oct 20, 2018

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

how about the simpler, more consistent rule:

always indent comment as follows:

some code[1 space][hash][1 space][comment]
const a = 1 # foo bar

as it stands, I find the hard-coded value of 31 for the comment to be inconsistent, since it only applies on certain AST nodes (eg after const a = 1) but not others (eg after proc testFoobar2() =)

proc testFoobar*(): auto = # foo2
  discard


proc testFoobar() =
  foobar(firstFar):
    const a = 1               #  foo2

proc testFoobar() = #foo1
  proc testFoobar2() = #    foo2
    foobar(firstFar):
      const a = 1             # foo2

proc testFoobar() = # foo1
  proc testFoobar2() = # foo2
    proc testFoobar2() = # foo2
      foobar(firstFar): # foo2
        const a = 1           # foo2
        let a2 = 1            # foo2
        a2.inc()              # foo2
        let a = echo: # foo
          echo "a"            # foo
        const asomevery_very_very_long_name = 1 # foo2

#foo1
# foo1
#   foo2
#   foo3

the above is invariant under nimpretty; but I'd like it to be transformed to:

proc testFoobar*(): auto = # foo2
  discard


proc testFoobar() =
  foobar(firstFar):
    const a = 1 # foo2

proc testFoobar() = # foo1
  proc testFoobar2() = # foo2
    foobar(firstFar):
      const a = 1 # foo2

proc testFoobar() = # foo1
  proc testFoobar2() = # foo2
    proc testFoobar2() = # foo2
      foobar(firstFar): # foo2
        const a = 1 # foo2
        let a2 = 1 # foo2
        a2.inc() # foo2
        let a = echo: # foo
          echo "a" # foo
        const asomevery_very_very_long_name = 1 # foo2

# foo1
# foo1
#   foo2
#   foo3

Note: 2 proposals here (both are independent)

  • always put exactly 1 space before # (when there is code on the left)
  • always put at least 1 space after # so that multiple spaces are left unchanged (useful to preserve indentation embedded inside a multiline comment), but 0 spaces will change to 1 space, eg:
# foo1
# foo1
#   foo2
#   foo3
@Araq

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

My own plan was to make the layouter produce tabs for this and then in a postprocessing step the tabs are aligned if that doesn't lead to too long lines.

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

unless I'm mis-understanding something in your plan, a problem with that is that a small code change can have non-local effects, resulting in large diffs; do you have a simple input/output example?
definitely will have this issue if you're aligning tabs globally.

a better strategy IMO is keep alignment local, where locality is restricted to a consecutive comments at same indentation level (IIRC that's what clang-format does, not 100% though):
so the following would result in nimpretty nooop:

proc fun() =
  const abc = 1    # comment 1
  const foobar = 1 # comment 1
  const x = 1      # comment 1

  const foo_abc = 1    # comment 1
  const foo_foobar = 1 # comment 1
  const foo_x = 1      # comment 1

the algorithm is 2-pass:

  • 1st pass computes alignment column in a locality (where locality is defined above)
  • 2nd pass uses that alignment
@Araq

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

Well "tabs are aligned" for consecutive lines, leading to what you've described. (I hope.)

@Araq Araq closed this in caf93f4 Jun 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.