cli, tests: write more things to stderr

[ErikSchierboom]

Closes https://github.com/exercism/configlet/issues/200

[ee7]

There's a few other places that I'd like to change in order to close #200.

What do you think about adding this commit to this PR? output-to-stderr...ee7:output-to-stderr

With that commit, searching for stdout:

git grep --break --heading 'stdout'

src/cli.nim
177:  let f = if exitCode == 0: stdout else: stderr

src/uuid/uuid.nim
6:  ## Writes `n` version 4 UUIDs to stdout. Writes only 1000 UUIDs if `n` is
17:  stdout.write s

and searching for stderr:

git grep --break --heading 'stderr'

src/cli.nim
177:  let f = if exitCode == 0: stdout else: stderr
188:  stderr.styledWrite(fgRed, "Error: ")
189:  stderr.write(s)
190:  stderr.write("\n\n")

src/helpers.nim
21:  stderr.styledWriteLine(fgRed, description & ":")
22:  stderr.writeLine(details)
23:  stderr.write "\n"

tests/test_binary.nim
23:    stderr.writeLine outp
274:  stderr.write(&"Running `{cmd}`... ")
277:    stderr.writeLine "success"
279:    stderr.writeLine "failure"

And searching for echo:

git grep --break --heading 'echo'

.github/bin/create-artifact
23:echo "ARTIFACT_FILE=${artifact_file}" >> "${GITHUB_ENV}"

.github/bin/publish-release
3:version="$(echo "${REF}" | cut -d'/' -f3)"

src/cli.nim
184:  echo &"{configletVersion}"

src/lint/lint.nim
47:  echo "The lint command is under development.\n" &
58:    echo """

src/sync/sync.nim
12:  echo &"""The following test case is missing:"
26:    echo "Unknown response. Skipping test case..."
32:  echo &"""The following test case is missing:"
49:    echo "Unknown response. Skipping test case..."

We can also consider:

• making our logger write to stderr (see docs for logging.newConsoleLogger)
• using our logger everywhere

But I think at least the latter is best saved for a later PR.

[ErikSchierboom]

I've opened #208 to make it easier to discuss. I've left two comments on that PR.

[ee7]

Another thing: when running configlet lint, shouldn't a message about a problem in e.g. config.json go to stdout, not stderr? If I wrote configlet lint > lint_output.txt I'd expect to see all the reported problems in that file.

Let's handle #204 first and then rebase this PR on top.

I've opened #208 to make it easier to discuss

Thanks.

[ErikSchierboom]

Another thing: when running configlet lint, shouldn't a message about a problem in e.g. config.json go to stdout, not stderr? If I wrote configlet lint > lint_output.txt I'd expect to see all the reported problems in that file.

🤔 I don't know how other CLI applications do this.

[ee7]

It's possible to argue that setFalseAndPrint should write to stdout because:

• we only use setFalseAndPrint for telling a user about problems found while running configlet lint
• the purpose of configlet lint is to produce such output
• a user might write configlet lint > foo.txt, expecting to see messages about linting problems in that file.
• a user might write e.g. configlet lint | grep -n1 'config.json', expecting to filter the output for problems that relate to a config.json file. It's more complicated to pipe stderr, e.g.:
  configlet lint 2>&1 >/dev/null | grep -n1 'config.json'

I don't know how other CLI applications do this.

It looks like shellcheckwrites its commentary to stdout, and I'd expect that its developers have thought about this issue more than most.

$ cat foo.sh

#!/bin/bash

a = 2

$ shellcheck foo.sh 2>/dev/null

In foo.sh line 3:
a = 2
^-- SC2034: a appears unused. Verify use (or export if used externally).
  ^-- SC1068: Don't put spaces around the = in assignments (or quote to make it literal).

For more information:
  https://www.shellcheck.net/wiki/SC1068 -- Don't put spaces around the = in ...
  https://www.shellcheck.net/wiki/SC2034 -- a appears unused. Verify use (or ...

But it might be more common for linters to write detected problems to stderr. For example: Rust's clippy.

I think there isn't always a right answer regarding stdout/stderr separation. I think either is fine for now. Maybe let's stick with stdout in setFalseAndPrint for now, save the "make configlet lint write detected problems to stderr" change for a separate PR?

For the current use of setFalseAndPrint, see:

$ git grep --heading --break 'setFalseAndPrint'

src/helpers.nim
20:proc setFalseAndPrint*(b: var bool; description: string; details: string) =

src/lint/concept_exercises.nim
51:            result.setFalseAndPrint("JSON parsing error", getCurrentExceptionMsg())

src/lint/lint.nim
17:          result.setFalseAndPrint("Missing file", path)

src/lint/track_config.nim
51:      result.setFalseAndPrint("Not a valid tag: " & $data, path)
53:    result.setFalseAndPrint("Tag is not a string: " & $data, path)
79:        result.setFalseAndPrint("JSON parsing error", getCurrentExceptionMsg())
84:    result.setFalseAndPrint("Missing file", configJsonPath)

src/lint/validators.nim
11:    result.setFalseAndPrint("Not an object: " & q(context), path)
17:      result.setFalseAndPrint("Not an object: " & q(key), path)
19:    result.setFalseAndPrint("Missing key: " & q(key), path)
27:        result.setFalseAndPrint("String is zero-length: " & q(key), path)
29:        result.setFalseAndPrint("String is whitespace-only: " & q(key), path)
31:      result.setFalseAndPrint("Not a string: " & q(key) & ": " & $data[key], path)
33:    result.setFalseAndPrint("Missing key: " & q(key), path)
49:          result.setFalseAndPrint("Array is empty: " & format(context, key), path)
55:              result.setFalseAndPrint("Array contains zero-length string: " &
58:              result.setFalseAndPrint("Array contains whitespace-only string: " &
61:            result.setFalseAndPrint("Array contains non-string: " &
64:      result.setFalseAndPrint("Not an array: " & format(context, key), path)
66:    result.setFalseAndPrint("Missing key: " & format(context, key), path)
76:          result.setFalseAndPrint("Array is empty: " & q(key), path)
82:      result.setFalseAndPrint("Not an array: " & q(key), path)
84:    result.setFalseAndPrint("Missing key: " & q(key), path)
90:      result.setFalseAndPrint("Not a bool: " & q(key) & ": " & $data[key], path)
92:    result.setFalseAndPrint("Missing key: " & q(key), path)
98:      result.setFalseAndPrint("Not an integer: " & q(key) & ": " & $data[key], path)
100:    result.setFalseAndPrint("Missing key: " & q(key), path)

[ErikSchierboom]

Maybe let's stick with stdout in setFalseAndPrint for now, save the "make configlet lint write detected problems to stderr" change for a separate PR?

Sounds good!

[ee7]

Maybe let's stick with stdout in setFalseAndPrint for now, save the "make configlet lint write detected problems to stderr" change for a separate PR?

Sounds good!

I've amended the first commit to exclude that change, and rebased on main.

I don't have a big preference about "squash and merge" vs "rebase and merge" here. But if we do squash then

console: write error messages to stderr

is a bad commit message heading, since now only 1 of the 3 commits is really about "error messages".

Happy to "rebase and merge" on the basis of preserving the specificity of the commit messages?

[ee7]

Approved for "rebase and merge".