lint: begin linting concept exercise config.json
#169
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ErikSchierboom
approved these changes
Feb 2, 2021
src/lint/concept_exercises.nim
Outdated
Comment on lines
30
to
35
| let j = | ||
| try: | ||
| parseFile(configPath) | ||
| except: | ||
| writeError("JSON parsing error", getCurrentExceptionMsg()) | ||
| continue |
There was a problem hiding this comment.
We can probably extract this to a helper function too as we'll have similar code for other files too.
There was a problem hiding this comment.
Agreed - let's do it after this PR. We can refactor this code to use the helper function too:
Lines 10 to 17 in 4849007
This code should be improved later too - it currently assumes that any exception raised during parseFile is a parsing error, even if it's an IOError.
ee7
force-pushed
the
lint-concept-exercise-configs
branch
from
12a9220
to
7d2e999
Compare
7 tasks
ee7
force-pushed
the
lint-concept-exercise-configs
branch
from
7d2e999
to
41f7e4c
Compare
|
I fixed some of the bugs. It might be worthwhile now to checkout this code locally and test it a bit. Note that this PR is still missing the rules that aren't purely schema-like checks. I'll suggest adding those in another PR if it doesn't end up being convenient to do it here. The error messages look particularly bad when there's more than one error in a single file. I'm leaning towards combining them per-file under that file's name. |
|
What should the exit code of |
For now, it should be a success exit code (0). Once we check to see if the concept directories and their config.json entries are in sync, we can error. |
|
Some example output with up-to-date track repos: bashcsharp/fsharp/elixirgojuliapythonrubyrust |
|
@ErikSchierboom And would you like to group output per-file like this? |
There was a problem hiding this comment.
Regarding the sample output, I'm not finding things out about my spec :)
bash
Directory does not exist:
exercises/concept
I think it's better if we don't show this because it is not really a requirement at the moment.
go
Array is empty: 'forked_from':
exercises/concept/conditionals/.meta/config.json
I think I was too harsh in the spec. We should allow an empty array for optional array properties. I've submitted a spec update PR.
Missing key: 'exercism_username':
exercises/concept/conditionals/.meta/config.json
The spec was outdated, this field should be optional. PR submitted
julia
Missing file:
exercises/concept/annalyns-infiltration/.docs/hints.md
There are a lot of missing files. These exercises are almost all in a wip state (which we've only recently added). I need to update the spec to indicate what to do in this case (probably a warning instead of an error).
Array is empty: 'contributors':
exercises/concept/encounters/.meta/config.json
See above comment on allowing an empty array for optional array properties.
Not an array: 'forked_from':
exercises/concept/encounters/.meta/config.json
This is a good catch!
src/lint/validators.nim
Outdated
Comment on lines
7
to
17
| proc isObject*(data: JsonNode, key: string, path: string, | ||
| isRequired = true): bool = | ||
| result = true | ||
| if key.len == 0: | ||
| if data.kind != JObject: | ||
| writeError("JSON root is not an object", path) | ||
| elif data.hasKey(key): | ||
| if data[key].kind != JObject: | ||
| writeError("Not an object: " & q(key), path) | ||
| elif isRequired: | ||
| writeError("Missing key: " & q(key), path) |
There was a problem hiding this comment.
I'm not sure about allowing the key parameter to be empty. I did not understand what the empty string did in if isObject(data, "", path):. I came up with two alternatives:
- Split this up into two functions
- Make the context a parameter with a default value
I personally prefer the second option, as the checkArrayOfStrings template has the same property (and empty context being specified).
src/lint/validators.nim
Outdated
Comment on lines
22
to
23
| if data[key].getStr().len == 0: | ||
| writeError("String is zero-length: " & q(key), path) |
There was a problem hiding this comment.
I noticed that the spec did not indicate that non-empty strings also not be non-blank (as in: not consist of only white space). I've just updated the spec. Could you update this check to reflect that the string not be empty after trimming?
src/lint/validators.nim
Outdated
Comment on lines
45
to
46
| elif item.getStr().len == 0: | ||
| writeError("Array contains zero-length string: " & format(context, key), path) |
There was a problem hiding this comment.
Same comment regarding non-blank strings.
The spec was updated. See: - exercism/docs@1de584652ba4
The spec was updated. See: - exercism/docs@7d29c2217f36
The spec was updated. See: - exercism/docs@e0336f96aa21
|
Pushed some new commits. I've run The first list looks suspiciously short, but I haven't checked how many tracks have merged the v3 PR. Tracks that exit with 1gojavajuliapythonrubyrustTracks that exit with 0Click to expand |
|
I hear that the v3 PRs have been merged for every track repo apart from Here's this PR's Tracks that exit with 1clojurecppdartgojavajuliakotlinpurescriptpythonrubyrustscalaswiftx86-64-assemblyTracks that exit with 0click to expand |
ErikSchierboom
approved these changes
Feb 5, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WIP proof-of-concept with templates, rather than a macro as done previously.
The error messages are currently bad. I'll write more later.