lint: refactor logic of finding missing files

[ee7]

Also remove an unneeded import and fix trivial style issues.

[ee7]

@ErikSchierboom can you review again? I tweaked it a bit to:

• Prefer proc over template
• Try to improve the function name
• Try to Improve the comments

[ErikSchierboom]

Should this be:

Suggested change

	writeError("Missing file", path)
	writeError("Missing file", path)
	result = false

[ee7]

No, because the current writeError is:

configlet/src/lint/lint.nim

Lines 4 to 8 in a7f090e

	template writeError(description: string, details: string) =
	stdout.styledWriteLine(fgRed, description & ":")
	stdout.writeLine(details)
	stdout.write "\n"
	result = false

Due to how templates work, writeError is already setting the result of the subdirsContain proc to false.

Try running configlet lint with this PR using some track dir that has missing files. You should see that we detect them, but the only place that can set result to false is inside the writeError template.

I might refactor this pattern later though. I haven't settled on the most expressive/maintainable way to write the linting code, but this is one alternative to writing something like:

proc isValidConfigFile(...): bool =
  result = true
  if not isValidFoo(...):
    result = false
  if not isValidBar(...):
    result = false

where we write result = false everywhere. Another alternative is to pass result as a var parameter to each validator function:

proc checkValidFoo(..., res: var bool) =
  if not myCond:
    res = false

proc isValidConfigFile(...): bool =
  result = true
  checkValidFoo(..., result)
  checkValidBar(..., result)

We can't write this

proc isValidConfigFile(...): bool =
  isValidFoo(...) and isValidBar(...) and isValidBaz(...)

because we don't want to short-circuit - we should show later linting errors even if earlier ones fail.

See the use of writeError in #169 - in particular, how the result of isValidConceptExerciseConfig can become false. I don't claim this is the best approach, though.

We could probably improve the writeError name - any ideas?

[ErikSchierboom]

Ah, now I understand. I think the original pattern is fine, but maybe with some rename as suggested. But what to use as the name... 🤔 The only thing I could come up with is handleError, but I don't know.

[ErikSchierboom]

A general question: when should one assign to result instead of doing return?

[ee7]

That's a good question. It's partly a matter of personal taste.

The official style guide contains:

The return statement should ideally be used when its control-flow properties are required. Use a procedure's implicit result variable whenever possible. This improves readability.

That's a common style, which optimizes for making immediate return obvious. However, Nim style is still evolving: some people argue that we should use return more often, and use result = only when building up a return value over multiple lines.

We also have the option of omitting result = or return entirely here, writing just

subdirsContain(conceptDir, conceptExerciseFiles)

The parent proc then returns the result of that call (the compiler enforces that it returns a bool as declared in the proc signature).

That style can improve readability/robustness in some places, but here I find it clearest to assign to result - we're doing the call because we're interested in its return value, rather than to perform some other action.

Let me know if I can clarify further.

You might like to read https://peterme.net/tips-and-tricks-with-implicit-return-in-nim.html - probably one of the best discussions on this topic.

[ErikSchierboom]

That does help! Thanks