lint: refactor validator templates to procs #204

ErikSchierboom · 2021-02-27T13:55:39Z

While the template-based validator functions lead to some really nice looking code, working with them proved to be slightly harder. To keep the barrier to new contributors as low as possible, the templates have been converted to regular procs, which are easier to understand by people new to Nim.

ErikSchierboom

With this PR, I've purposefully tried to make the code as obvious as possible. This will allow us to more easily add linting rules. I'll leave it to you (@ee7) to check if we can refactor this to something prettier without losing any readability. But I think this PR gets us to a state where we can at least make good progress on adding new linting rules.

ErikSchierboom · 2021-02-27T13:59:54Z

src/lint/concept_exercises.nim

+    if not checkString(data, "github_username", path):
+      result = false
+    if not checkString(data, "exercism_username", path, isRequired = false):
+      result = false


According to the documentation, the and operator does short-cut evaluation. Otherwise I might have written it like this:

Suggested change

if not checkString(data, "github_username", path):

result = false

if not checkString(data, "exercism_username", path, isRequired = false):

result = false

result =

checkString(data, "github_username", path) and

checkString(data, "exercism_username", path, isRequired = false)

Another option I've considered was to pass the result value as an argument to the checkString function, but the current, verbose code does have the advantage of being very easy to understand.

If the and short circuit the first operation, we could do:

result = true [...] result = checkString(data, "github_username", path) and result result = checkString(data, "exercism_username", path) and result

It's more concise and maybe prettier, but possibly less obvious for someone less seasoned.

That is an option. I'll wait for @ee7 to weigh in too

Another option I've considered was to pass the result value as an argument to the checkString function

Yes, my first preference would probably be:

result = true result.foo(data, "github_username", path) result.foo(data, "exercism_username", path, isRequired = false)

if we can find a name for foo that conveys "set result to false if"

Because this seems less obvious:

result = true data.checkString("github_username", path, result) data.checkString("exercism_username", path, result, isRequired = false)

My second preference would probably be as @valentin-p suggested.

The and operator does work like that:

proc echoAndReturnTrue: bool = echo "t" return true proc echoAndReturnFalse: bool = echo "f" return false proc foo: bool = result = true result = echoAndReturnTrue() and result result = echoAndReturnFalse() and result result = echoAndReturnTrue() and result result = echoAndReturnFalse() and result proc bar: bool = result = true result = result and echoAndReturnTrue() result = result and echoAndReturnFalse() result = result and echoAndReturnTrue() result = result and echoAndReturnFalse() echo foo() echo "" echo bar()

t f t f false t f false

ErikSchierboom · 2021-02-27T14:03:00Z

src/lint/concept_exercises.nim

+      result = false
+    if not checkFiles(data, "files", path):
+      result = false
+    if not checkArrayOfStrings(data, "", "forked_from", path, isRequired = false):


I still don't like the "" argument being required. I'd like to refactor in the future to not having to require that. But it's fine for now.

ee7 · 2021-03-03T04:17:14Z

Updated some writeError that weren't amended with the new definition - each writeError should be followed by result = false or return false.

As of bb0bed1:

$ git grep --ignore-case --break --heading --context=1 writeError

src/helpers.nim
19-
20:proc writeError*(description: string, details: string) =
21-  stdout.styledWriteLine(fgRed, description & ":")

src/lint/concept_exercises.nim
49-          except:
50:            writeError("JSON parsing error", getCurrentExceptionMsg())
51-            result = false

src/lint/lint.nim
16-        if not fileExists(path):
17:          writeError("Missing file", path)
18-          result = false

src/lint/track_config.nim
50-    if not tags.contains(s):
51:      writeError("Not a valid tag: " & $data, path)
52-      result = false
53-  else:
54:    writeError("Tag is not a string: " & $data, path)
55-    result = false
--
80-      except:
81:        writeError("JSON parsing error", getCurrentExceptionMsg())
82-        return false
--
85-  else:
86:    writeError("Missing file", configJsonPath)
87-    result = false

src/lint/validators.nim
10-  if data.kind != JObject:
11:    writeError("Not an object: " & q(context), path)
12-    result = false
--
18-    if data[key].kind != JObject:
19:      writeError("Not an object: " & q(key), path)
20-      result = false
21-  elif isRequired:
22:    writeError("Missing key: " & q(key), path)
23-    result = false
--
30-      if s.len == 0:
31:        writeError("String is zero-length: " & q(key), path)
32-        result = false
33-      elif s.strip().len == 0:
34:        writeError("String is whitespace-only: " & q(key), path)
35-        result = false
36-    else:
37:      writeError("Not a string: " & q(key) & ": " & $data[key], path)
38-      result = false
39-  elif isRequired:
40:    writeError("Missing key: " & q(key), path)
41-    result = false
--
55-        if isRequired:
56:          writeError("Array is empty: " & format(context, key), path)
57-          result = false
--
62-            if s.len == 0:
63:              writeError("Array contains zero-length string: " & format(context, key), path)
64-              result = false
65-            elif s.strip().len == 0:
66:              writeError("Array contains whitespace-only string: " & q(key), path)
67-              result = false
68-          else:
69:            writeError("Array contains non-string: " & format(context, key) & ": " & $item, path)
70-            result = false
71-    else:
72:      writeError("Not an array: " & format(context, key), path)
73-      result = false
74-  elif isRequired:
75:    writeError("Missing key: " & format(context, key), path)
76-    result = false
--
85-        if isRequired:
86:          writeError("Array is empty: " & q(key), path)
87-          result = false
--
92-    else:
93:      writeError("Not an array: " & q(key), path)
94-      result = false
95-  elif isRequired:
96:    writeError("Missing key: " & q(key), path)
97-    result = false
--
102-    if data[key].kind != JBool:
103:      writeError("Not a bool: " & q(key) & ": " & $data[key], path)
104-      result = false
105-  elif isRequired:
106:    writeError("Missing key: " & q(key), path)
107-    result = false
--
112-    if data[key].kind != JInt:
113:      writeError("Not an integer: " & q(key) & ": " & $data[key], path)
114-      result = false
115-  elif isRequired:
116:    writeError("Missing key: " & q(key), path)
117-    result = false

ee7 · 2021-03-03T04:43:31Z

I don't love the repetition of result = false after every writeError.

We could make writeError take a var bool as the first parameter, and rename to something like setFalse:

proc setFalse*(b: var bool, description: string, details: string) =
  b = false 
  stdout.styledWriteLine(fgRed, description & ":")
  stdout.writeLine(details)
  stdout.write "\n"

I've pushed a branch that implements that, one commit ahead of this PR. See bb0bed1...1f619dd

What do you think?

ErikSchierboom · 2021-03-03T08:14:53Z

I don't love the repetition of result = false after every writeError.

We could make writeError take a var bool as the first parameter, and rename to something like setFalse:

What do you think?

I like the pattern. The only thing I don't like is the name, as we are not just setting it to false but also writing an error. I don't really know of any better names, but I feel like we should at least mention that it is doing something with error handling.

setFalseAndWriteError
setFalseWithError
setError
updateFromError
updateWithError

If we can find a better name, I'm happy to use your version.

ee7 · 2021-03-03T10:55:31Z

The only thing I don't like is the name, as we are not just setting it to false but also writing an error. I don't really know of any better names, but I feel like we should at least mention that it is doing something with error handling.

Agreed.

I'm fine with setFalseAndWriteError, although it's a bit long. Good enough for now?

If linting errors will be written to stdout, maybe there's a better setFalseAndVerb that avoids the word "error" - also clarifying that it's "a problem that configlet lint found on a track" not a "configlet error". For example:

setFalseAndEcho
setFalseAndOutput
setFalseAndWarn
setFalseAndNotify

ErikSchierboom · 2021-03-03T11:29:00Z

I'm fine with setFalseAndWriteError, although it's a bit long. Good enough for now?

Yeah, agreed. I'm also fine with setFalseAndEcho, setFalseAndOutput and setFalseAndNotify. Your pick :) Let me know when you've updated your branch, then I'll merge them into this PR.

ee7 · 2021-03-04T00:50:07Z

Let me know when you've updated your branch

Updated the branch, see bb0bed1...abc07a8. I went with setFalseAndPrint for now, if you're okay with that.

Bikeshedding:

setFalseAndWriteError - pretty long, and "error" could be confusing
setFalseAndEcho - okay, but we don't actually use echo
setFalseAndOutput - "output" is also a noun, so this can be misread as "set false and set output"
setFalseAndNotify - "notify" is more vague than "print"

I also added a commit that makes some style changes for lines we touched.

Feel free to push those commits to this PR if you approve of them.

I've verified that the output of configlet lint is the same for every track with the current main (d4eee1f) and the tip of that branch (abc07a8). The output of configlet lint as of the time of this post is below:

Tracks that exit with 1

click to expand

05ab1e

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

ada

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

arm64-assembly

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

babashka

Array is empty: 'tags':
./config.json

ballerina

Array is empty: 'tags':
./config.json

ceylon

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

cfml

Array is empty: 'tags':
./config.json

clojure

Array is empty: 'tags':
./config.json

Missing file:
./exercises/concept/bird-watcher/.docs/hints.md

Missing file:
./exercises/concept/cars-assemble/.docs/hints.md

Missing file:
./exercises/concept/interest-is-interesting/.docs/hints.md

Missing file:
./exercises/concept/interest-is-interesting/.meta/config.json

Missing file:
./exercises/concept/international-calling-connoisseur/.docs/hints.md

Missing file:
./exercises/concept/international-calling-connoisseur/.docs/introduction.md

Missing file:
./exercises/concept/international-calling-connoisseur/.meta/config.json

Missing file:
./exercises/concept/log-levels/.docs/hints.md

Missing file:
./exercises/concept/squeaky-clean/.docs/hints.md

Missing file:
./exercises/concept/squeaky-clean/.docs/introduction.md

Missing file:
./exercises/concept/squeaky-clean/.meta/config.json

Array is empty: 'files.solution':
./exercises/concept/annalyns-infiltration/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/annalyns-infiltration/.meta/config.json

Array is empty: 'files.solution':
./exercises/concept/bird-watcher/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/bird-watcher/.meta/config.json

Array is empty: 'files.exemplar':
./exercises/concept/bird-watcher/.meta/config.json

Array is empty: 'files.solution':
./exercises/concept/cars-assemble/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/cars-assemble/.meta/config.json

Array is empty: 'files.solution':
./exercises/concept/log-levels/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/log-levels/.meta/config.json

Array is empty: 'files.solution':
./exercises/concept/lucians-luscious-lasagna/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/lucians-luscious-lasagna/.meta/config.json

Missing key: 'authors':
./exercises/concept/tracks-on-tracks-on-tracks/.meta/config.json

Array is empty: 'files.solution':
./exercises/concept/tracks-on-tracks-on-tracks/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/tracks-on-tracks-on-tracks/.meta/config.json

clojurescript

Array is empty: 'tags':
./config.json

coffeescript

Array is empty: 'tags':
./config.json

coq

Array is empty: 'tags':
./config.json

cpp

Array is empty: 'tags':
./config.json

Array is empty: 'files.solution':
./exercises/concept/strings/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/strings/.meta/config.json

crystal

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

d

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

dart

Array is empty: 'tags':
./config.json

Missing file:
./exercises/concept/futures/.meta/config.json

Missing file:
./exercises/concept/numbers/.meta/config.json

Missing file:
./exercises/concept/strings/.meta/config.json

emacs-lisp

Array is empty: 'tags':
./config.json

erlang

Array is empty: 'tags':
./config.json

factor

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

forth

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

fortran

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

gleam

Array is empty: 'tags':
./config.json

gnu-apl

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

go

Array is empty: 'files.test':
./exercises/concept/comments/.meta/config.json

Array is empty: 'files.solution':
./exercises/concept/constants/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/constants/.meta/config.json

Array is empty: 'files.solution':
./exercises/concept/errors/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/errors/.meta/config.json

Array is empty: 'files.exemplar':
./exercises/concept/errors/.meta/config.json

groovy

Array is empty: 'tags':
./config.json

haskell

Array is empty: 'tags':
./config.json

idris

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

io

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

j

Array is empty: 'tags':
./config.json

java

Array is empty: 'tags':
./config.json

julia

Missing file:
./exercises/concept/annalyns-infiltration/.docs/hints.md

Missing file:
./exercises/concept/annalyns-infiltration/.docs/introduction.md

Missing file:
./exercises/concept/annalyns-infiltration2/.docs/hints.md

Missing file:
./exercises/concept/annalyns-infiltration2/.docs/introduction.md

Missing file:
./exercises/concept/emoji-times/.docs/introduction.md

Missing file:
./exercises/concept/exercism-matrix/.docs/hints.md

Missing file:
./exercises/concept/vehicle-purchase/.docs/hints.md

Missing file:
./concepts/abstract-types/introduction.md

Missing file:
./concepts/composite-types/introduction.md

Missing file:
./concepts/constants/introduction.md

Missing file:
./concepts/matrices-concatenation/introduction.md

Missing file:
./concepts/matrices-indices/introduction.md

Missing file:
./concepts/matrices-iteration/introduction.md

Missing file:
./concepts/matrices-mutation/introduction.md

Missing file:
./concepts/methods/introduction.md

Missing file:
./concepts/symbols/introduction.md

Not an array: 'forked_from':
./exercises/concept/annalyns-infiltration/.meta/config.json

Not an array: 'forked_from':
./exercises/concept/annalyns-infiltration2/.meta/config.json

Not an array: 'forked_from':
./exercises/concept/elyses-enchantments/.meta/config.json

Not an array: 'forked_from':
./exercises/concept/emoji-times/.meta/config.json

Not an array: 'forked_from':
./exercises/concept/encounters/.meta/config.json

Not an array: 'forked_from':
./exercises/concept/exercism-matrix/.meta/config.json

Not an array: 'forked_from':
./exercises/concept/fibonacci-iterator/.meta/config.json

Not an array: 'forked_from':
./exercises/concept/lasagna/.meta/config.json

Not an array: 'forked_from':
./exercises/concept/leap/.meta/config.json

Not an array: 'forked_from':
./exercises/concept/stage-heralding/.meta/config.json

Not an array: 'forked_from':
./exercises/concept/vehicle-purchase/.meta/config.json

kotlin

Array is empty: 'tags':
./config.json

Array is empty: 'files.solution':
./exercises/concept/annalyns-infiltration/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/annalyns-infiltration/.meta/config.json

Array is empty: 'files.exemplar':
./exercises/concept/annalyns-infiltration/.meta/config.json

Array is empty: 'files.solution':
./exercises/concept/basics/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/basics/.meta/config.json

Array is empty: 'files.exemplar':
./exercises/concept/basics/.meta/config.json

lfe

Array is empty: 'tags':
./config.json

mips

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

nix

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

objective-c

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

ocaml

Array is empty: 'tags':
./config.json

perl5

Array is empty: 'tags':
./config.json

pharo-smalltalk

Array is empty: 'tags':
./config.json

plsql

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

pony

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

powershell

Array is empty: 'tags':
./config.json

prolog

Array is empty: 'tags':
./config.json

purescript

Array is empty: 'tags':
./config.json

Array is empty: 'files.solution':
./exercises/concept/booleans/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/booleans/.meta/config.json

racket

Array is empty: 'tags':
./config.json

raku

Array is empty: 'tags':
./config.json

reasonml

Array is empty: 'tags':
./config.json

research_experiment_1

Missing key: 'slug':
./config.json

String is zero-length: 'blurb':
./config.json

Missing key: 'version':
./config.json

Missing key: 'tags':
./config.json

ruby

Array is empty: 'tags':
./config.json

scala

Array is empty: 'tags':
./config.json

Array is empty: 'files.solution':
./exercises/concept/basics/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/basics/.meta/config.json

scheme

Array is empty: 'tags':
./config.json

shen

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

sml

Array is empty: 'tags':
./config.json

solidity

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

swift

Array is empty: 'tags':
./config.json

Missing file:
./concepts/capturing/introduction.md

Missing file:
./concepts/characters/about.md

Missing file:
./concepts/characters/links.json

Missing file:
./concepts/classes/about.md

Missing file:
./concepts/classes/links.json

Missing file:
./concepts/closures/introduction.md

Missing file:
./concepts/constants-and-variables/introduction.md

Missing file:
./concepts/initializers/about.md

Missing file:
./concepts/initializers/links.json

Missing file:
./concepts/opaque-indices/about.md

Missing file:
./concepts/opaque-indices/links.json

Missing file:
./concepts/shorthand-arguments/introduction.md

Missing file:
./concepts/strings/about.md

Missing file:
./concepts/strings/links.json

Missing file:
./concepts/structs/about.md

Missing file:
./concepts/structs/links.json

Missing file:
./concepts/trailing-closures/introduction.md

system-verilog

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

typescript

Array is empty: 'tags':
./config.json

vbnet

Array is empty: 'tags':
./config.json

vimscript

Array is empty: 'tags':
./config.json

x86-64-assembly

Array is empty: 'tags':
./config.json

Array is empty: 'files.solution':
./exercises/concept/basics/.meta/config.json

Array is empty: 'files.test':
./exercises/concept/basics/.meta/config.json

zig

String is zero-length: 'blurb':
./config.json

Array is empty: 'tags':
./config.json

Tracks that exit with 0

click to expand

bash
c
common-lisp
csharp
delphi
elixir
elm
fsharp
haxe
javascript
lua
nim
php
python
r
rust
tcl

Partly from running `nimpretty`.

ErikSchierboom · 2021-03-04T07:28:03Z

@ee7 I like setFalseAndPrint. I've merged your changes into this PR. Feel free to approve and merge.

ErikSchierboom · 2021-03-04T15:36:57Z

Thanks!

We should have removed this line in 1bb5c11 (exercism#204).

We should have removed this line in 1bb5c11 (#204). This was our only `export` in the whole codebase.

ErikSchierboom commented Feb 27, 2021

View reviewed changes

ErikSchierboom requested a review from ee7 February 27, 2021 14:05

lint: update other writeError call sites

bb0bed1

ee7 mentioned this pull request Mar 3, 2021

cli, tests: write more things to stderr #205

Merged

ee7 added 2 commits March 4, 2021 08:22

refactor: change writeError to setFalseAndPrint

82bac68

lint(validators): improve style

e0b4396

Partly from running `nimpretty`.

ee7 approved these changes Mar 4, 2021

View reviewed changes

ee7 merged commit 1bb5c11 into main Mar 4, 2021

ee7 deleted the templates-to-procs branch March 4, 2021 14:31

ee7 added a commit to ee7/exercism-configlet that referenced this pull request Mar 11, 2021

lint(validators): remove now-unused export

6108a5a

We should have removed this line in 1bb5c11 (exercism#204).

ee7 mentioned this pull request Mar 11, 2021

lint(validators): remove now-unused export #211

Merged

ee7 added a commit that referenced this pull request Mar 11, 2021

lint(validators): remove now-unused export (#211)

3915501

We should have removed this line in 1bb5c11 (#204). This was our only `export` in the whole codebase.

ee7 changed the title ~~refactor: replace validator templates with procs~~ lint: refactor validator templates to procs Mar 11, 2021

lint: refactor validator templates to procs #204

lint: refactor validator templates to procs #204

Conversation

ErikSchierboom commented Feb 27, 2021 • edited Loading

ErikSchierboom left a comment

Choose a reason for hiding this comment

ErikSchierboom Feb 27, 2021

Choose a reason for hiding this comment

valentin-p Mar 1, 2021

Choose a reason for hiding this comment

ErikSchierboom Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

ee7 Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

ErikSchierboom Feb 27, 2021

Choose a reason for hiding this comment

ee7 commented Mar 3, 2021 • edited Loading

ee7 commented Mar 3, 2021 • edited Loading

ErikSchierboom commented Mar 3, 2021

ee7 commented Mar 3, 2021

ErikSchierboom commented Mar 3, 2021

ee7 commented Mar 4, 2021 • edited Loading

Tracks that exit with 1

05ab1e

ada

arm64-assembly

babashka

ballerina

ceylon

cfml

clojure

clojurescript

coffeescript

coq

cpp

crystal

d

dart

emacs-lisp

erlang

factor

forth

fortran

gleam

gnu-apl

go

groovy

haskell

idris

io

j

java

julia

kotlin

lfe

mips

nix

objective-c

ocaml

perl5

pharo-smalltalk

plsql

pony

powershell

prolog

purescript

racket

raku

reasonml

research_experiment_1

ruby

scala

scheme

shen

sml

solidity

swift

system-verilog

typescript

vbnet

ErikSchierboom commented Feb 27, 2021 •

edited

Loading

ErikSchierboom Mar 2, 2021 •

edited

Loading

ee7 Mar 3, 2021 •

edited

Loading

ee7 commented Mar 3, 2021 •

edited

Loading

ee7 commented Mar 3, 2021 •

edited

Loading

ee7 commented Mar 4, 2021 •

edited

Loading