init wizard creates files with trailing whitespace

[todd-a-jacobs]

Describe the bug When running lucky init, the wizard creates a number of files with trailing whitespace. With the standard defaults for core.whitespace in Git v2.36.1 (and likely other versions) attempting to commit the results of lucky init will generate a number of errors.

To Reproduce

1. lucky init
2. Complete the wizard for project foo.
3. cd foo
4. git add .
5. git commit -av

This will result in errors from Git's default core.whitespace, and returns the following:

.github/workflows/ci.yml:115: trailing whitespace.
+        run: crystal spec 
docker/development.dockerfile:36: new blank line at EOF.
docker/wait-for-it.sh:189: new blank line at EOF.
script/system_check:7: trailing whitespace.
+# A few helper functions are provided to make writing bash a little easier. See the 
script/system_check:36: new blank line at EOF.

Expected behavior The default templates created by the wizard should not have:

1. trailing whitespace at the end of lines, or
2. blank lines at the end of files

Versions (please complete the following information):

• Lucky version (check in shard.lock): 0.30.0

• Crystal version (crystal --version):

  Crystal 1.4.1 (2022-04-22)
  
  LLVM: 14.0.5
  Default target: x86_64-apple-macosx

• OS: macOS 11.6.7

Additional context

Git whitespace options, both default and custom:

# returns nothing; it's using the defaults
$ git config --get core.whitespace

Git whitespace-related config options:

$ git config -l | fgrep whitespace
apply.whitespace=fix

Relevant ~/.gitconfig options:

[apply]
        whitespace = fix
[core]
        autocrlf     = input
        editor       = /usr/bin/env vim
        excludesfile = ~/.gitignore
        pager        = /usr/bin/less
        safecrlf     = true

Enabled .git/hooks/pre-commit hook within the project directory (doesn't test for whitespace issues):

#!/bin/sh
#
# An example hook script to verify what is about to be committed.
# Called by "git commit" with no arguments.  The hook should
# exit with non-zero status after issuing an appropriate message if
# it wants to stop the commit.
#
# To enable this hook, rename this file to "pre-commit".

if git rev-parse --verify HEAD >/dev/null 2>&1
then
    against=HEAD
else
    # Initial commit: diff against an empty tree object
    against=$(git hash-object -t tree /dev/null)
fi

# If you want to allow non-ASCII filenames set this variable to true.
allownonascii=$(git config --type=bool hooks.allownonascii)

# Redirect output to stderr.
exec 1>&2

# Cross platform projects tend to avoid non-ASCII filenames; prevent
# them from being added to the repository. We exploit the fact that the
# printable range starts at the space character and ends with tilde.
if [ "$allownonascii" != "true" ] &&
    # Note that the use of brackets around a tr range is ok here, (it's
    # even required, for portability to Solaris 10's /usr/bin/tr), since
    # the square bracket bytes happen to fall in the designated range.
    test $(git diff --cached --name-only --diff-filter=A -z $against |
      LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0
then
    cat <<\EOF
Error: Attempt to add a non-ASCII file name.

This can cause problems if you want to work with people on other platforms.

To be portable it is advisable to rename the file.

If you know what you are doing you can disable this check using:

  git config hooks.allownonascii true
EOF
    exit 1
fi

# If there are whitespace errors, print the offending file names and fail.
exec git diff-index --check --cached $against --

[jwoertink]

That's interesting. I've always intentionally added the trailing whitespace. That's been convention on all of my code :sweat_smile:

[stephendolan]

@jwoertink / @todd-a-jacobs I'd recommend that this issue gets taken up with the Ameba team since we use their default rules and they expect a trailing blank line (I think):

• https://crystal-ameba.github.io/ameba/Ameba/Rule/Layout/TrailingBlankLines.html
• https://github.com/crystal-ameba/ameba/blob/8a12229116f32b8520964e4f87eb390d56a6c765/spec/ameba/rule/layout/trailing_blank_lines_spec.cr

This certainly seems like a valid reason to change the default, but I think the Ameba team will have more knowledge about the trade-offs.

[jwoertink]

Good call. Thanks @stephendolan

[robacarp]

A lot of folks insist on adding a blank line to the end of files, you're not alone in that. I can't find an example but I'm sure that I've seen github drop an emoji in a diff which doesn't have a final blank line in it.

Git 2.36.1 still warns you at some points too, so it seems like the ecosystem is probably not consistent even within git. I got this warning today from a git commit -p workflow: "\ No newline at end of file"

Edit: see also https://stackoverflow.com/questions/2287967/why-is-it-recommended-to-have-empty-line-in-the-end-of-a-source-file

I don't think it really matters to me, except for using as many sane defaults as possible. If people are going to get nagged about it when they start from scratch install of git and lucky, it's worth changing.

[todd-a-jacobs]

I'd recommend that this issue gets taken up with the Ameba team since we use their default rules and they expect a trailing blank line (I think).

From what I can tell, the default rule is to have a trailing newline character (e.g. the final line ends with \n) rather than having trailing empty lines. This seems to be either something tweaked in the Ameba rules configured for the project, or someone just decided to add them despite the fact that trailing blank lines should be triggering an Ameba error.

[stephendolan]

All of the default Lucky content comes from here, with Ameba running via GitHub Actions on each PR: https://github.com/luckyframework/lucky_cli

It doesn't seem to be failing currently, but if you'd like to submit a PR to that repo to tweak this, I don't think anyone would be opposed to merging if it passes the default Ameba linter!

[jwoertink]

Looking at that Ameba linter, that's an opt-in feature, not opt-out it seems. So if you didn't want that, you'd create the ameba .ameba.yml file, and configure it there. I don't think we will change this on our end unless Ameba does decide to make this a default.

I'm gonna close this out, but if anyone has a solid reason for the benefits of changing this, I'm still open to discussion.