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 should hardcode indentation amount to 2 spaces #9502

Closed
timotheecour opened this Issue Oct 25, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@timotheecour
Copy link
Collaborator

timotheecour commented Oct 25, 2018

nimpretty currently applies to rest of file the 1st indentation it finds; in following case it uses an indentation of 1 space instead of usual 2:

proc foo1() =
 echo "bar"

proc foo2() =
  echo "bar"

nimpretty output:

proc foo1() =
 echo "bar"

proc foo2() =
 echo "bar"

I understand this may be by design but is that a good idea? especially if nimpretty is meant as a 0-config indentation tool? that'll lead to potential fragmentation of styles and larger diffs down the line, eg: if a user adds a different indentation as the very 1st proc in a file, the next time nimpretty is applied, a large diff will result (changing indentation everywhere in the file).

proposal

hardcode indentation of 2 spaces, used throughout Nim codebase and large majority of nimble packages
I don't see any disadvantage to using 2 spaces

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Oct 25, 2018

It is by design, plenty of people really prefer 4 spaces and since it can still be done with zero configuration it fits nimpretty's "philosophy".

@timotheecour

This comment has been minimized.

Copy link
Collaborator

timotheecour commented Oct 25, 2018

the problem is when it's by accident, because the 1st indentation is wrong; an alternative is to allow configuring via --indentSpaces:2 ; yes it's not zero configuration, but:

  • it helps in those 'accidental' cases ( i had some in my code base)
  • it helps enforcing a per-project guideline (eg Nim repo, which uses 2 spaces), eg we could have a git hook or CI to enforce that nimpretty(code after PR)=code after PR, after a 1-time PR that nimformats everything
    (the alternative is git nimformat analog to git clang-format)
  • it helps when importing foreign code from another project that could have different indentation
@Araq

This comment has been minimized.

Copy link
Member

Araq commented Oct 25, 2018

But instead of --indentSpaces:2 how hard is it to get the first indentation level right?

@timotheecour

This comment has been minimized.

Copy link
Collaborator

timotheecour commented Oct 25, 2018

true for a single file; not so if you either have lots of files (or if they're auto-generated somehow); I mean it's a trivial change to support --indentSpaces:2 option (and happy to add it myself in a PR)

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Oct 25, 2018

Alright...

@Araq Araq added Feature and removed RFC labels Oct 25, 2018

@b3liever

This comment has been minimized.

Copy link
Contributor

b3liever commented Oct 25, 2018

Come on its fine as it is...

@timotheecour timotheecour self-assigned this Oct 25, 2018

timotheecour added a commit to timotheecour/Nim that referenced this issue Oct 25, 2018

timotheecour added a commit to timotheecour/Nim that referenced this issue Oct 25, 2018

timotheecour added a commit to timotheecour/Nim that referenced this issue Oct 29, 2018

@Araq Araq closed this in d1fe195 Nov 11, 2018

narimiran added a commit that referenced this issue Nov 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment