Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lint: add initial linting of practice exercises #219

Merged
merged 3 commits into from
Mar 17, 2021
Merged

lint: add initial linting of practice exercises #219

merged 3 commits into from
Mar 17, 2021

Conversation

ErikSchierboom
Copy link
Member

This PR adds basic validation checks for practice exercises:

  • Check if the required files are present
  • Check if the .meta/config.json files are valid

The PR also applies a minor refactoring, where required file checking is moved to the corresponding module instead of the lint module.

The helper proc to check whether required files exist in a directory has been moved to the validators module to allow being called from the different modules.

ErikSchierboom
ErikSchierboom commented Mar 15, 2021
View reviewed changes
Copy link
Member Author

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is quite a bit of overlap between the practice exercises and concept exercises code. I'm usually hesitant to deduplicate code that is not entirely the same though, which is why I've not yet extracted any common functionality.

src/lint/practice_exercises.nim
import ".."/helpers
import "."/validators

proc isValidAuthorOrContributor(data: JsonNode, context: string, path: string): bool =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is currently duplicated between the practice_exercises and concept_exercises modules. We could extract it to some shared module.

src/lint/practice_exercises.nim
if not checkString(data, "exercism_username", path, isRequired = false):
result = false

proc checkFiles(data: JsonNode, context, path: string): bool =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is virtually identical to the corresponding concept exercise code, except for exemplar being named example here. We could extract this functionality into a shared helper.

src/lint/practice_exercises.nim
else:
result = false

proc isValidPracticeExerciseConfig(data: JsonNode, path: string): bool =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again, this logic is almost identical to the concept exercises' logic. For now, the only difference is that there is no forked_from property for practice exercises. There will be more difference later when more practice exercise checks are implemented. We could consider extracting the shared logic to a helper.

src/lint/practice_exercises.nim
if not checkString(data, "language_versions", path, isRequired = false):
result = false

proc isEveryPracticeExerciseConfigValid*(trackDir: string): bool =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And again, this logic is very similar to the concept exercises logic. We could extract it to a helper method.

@ee7
Copy link
Member

ee7 commented Mar 17, 2021

Looks good.

My main observation: I notice that this PR will fill the configlet lint output on many tracks with e.g.:

Array is empty: 'authors':
./exercises/practice/two-fer/.meta/config.json

Array is empty: 'files.solution':
./exercises/practice/two-fer/.meta/config.json

Array is empty: 'files.test':
./exercises/practice/two-fer/.meta/config.json

Array is empty: 'files.example':
.exercises/practice/two-fer/.meta/config.json

These 12 lines multiplied by the number of practice exercises is typically a pretty large amount of output.

And even on the tracks that have merged the files PRs it typically looks like:

Array is empty: 'authors':
./exercises/practice/accumulate/.meta/config.json

Array is empty: 'authors':
./exercises/practice/acronym/.meta/config.json

Array is empty: 'authors':
./exercises/practice/affine-cipher/.meta/config.json

...etc

Questions:

  1. Should we temporarily disable the files checking for practice exercises, until more tracks merge their files PRs?
  2. Should we temporarily disable the authors checking for practice exercises, perhaps until we send the PRs that add authors?

I'm fine with "no" for both. But if "no" for question 2, do we want to tell users not to add authors themselves? I assume that we prefer to do it via a mass PR.

See below for the comparison before and after this PR.

Before this PR

Tracks that exit with 0

bash
c
common-lisp
csharp
delphi
elixir
elm
erlang
fsharp
go
haxe
java
javascript
lua
nim
pharo-smalltalk
php
python
r
rust
tcl
zig

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

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

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

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

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

With this PR

The output across every track is too long (30,000 lines) to post here.

See https://gist.github.com/ee7/100ce3fa79310ca7ec7845eb21c13b4f

@ErikSchierboom
Copy link
Member Author

Should we temporarily disable the files checking for practice exercises, until more tracks merge their files PRs?

I'm in doubt. Few tracks have updated the file pattern: exercism/v3-launch#19 That would indeed mean lots of linting errors. This is both good and bad. Good in that tracks will be confronted with the fact that they have to do this, bad in that we'll "break" a lot of tracks. I've just posted a message on the v3 Slack channel requesting people to update their file patterns. Let's temporarily disable this rule and enable it later on.

Should we temporarily disable the authors checking for practice exercises, perhaps until we send the PRs that add authors?

This is probably good. I am still (intermittently) working on my script to bulk PR the authors, but we can disable those checks for now.

@ErikSchierboom
Copy link
Member Author

@ee7 I've added a commit to temporarily disable the authors and files checks.

@ee7
Copy link
Member

ee7 commented Mar 17, 2021

OK, sounds good. The change that this PR now makes to the configlet lint output is below.

Tracks that no longer exit with 0:

- erlang
- pharo-smalltalk
- r
- rust

Diff (it's messy because it has the output for every track in one file, but it should give the idea):

@@ -85,6 +81,30 @@ Array is empty: 'tags':
 Array is empty: 'tags':
 ./config.json
+
+Missing file:
+./exercises/practice/calculator-service/.docs/instructions.md
+
+Missing file:
+./exercises/practice/echo-service/.docs/instructions.md
+
+Missing file:
+./exercises/practice/greeting-service/.docs/instructions.md
+
+Missing file:
+./exercises/practice/hello-world-service/.docs/instructions.md
+
+Missing file:
+./exercises/practice/legacy-service-client/.docs/instructions.md
+
+Missing file:
+./exercises/practice/order-management/.docs/instructions.md
+
+Missing file:
+./exercises/practice/service-composition/.docs/instructions.md
+
+Missing file:
+./exercises/practice/service-invocation/.docs/instructions.md



@@ -223,6 +243,9 @@ Array is empty: 'tags':
 Array is empty: 'tags':
 ./config.json
+
+Missing file:
+./exercises/practice/tautology/.docs/instructions.md



@@ -300,6 +323,16 @@ Array is empty: 'tags':



+#### erlang
+Missing file:
+./exercises/practice/bracket-push/.docs/instructions.md


@@ -577,6 +610,9 @@ String is zero-length: 'blurb':

 Array is empty: 'tags':
 ./config.json
+
+Missing file:
+./exercises/practice/bracket-push/.docs/instructions.md


@@ -603,6 +639,16 @@ Array is empty: 'tags':



+#### pharo-smalltalk
+Missing file:
+./exercises/practice/die/.docs/instructions.md


@@ -654,6 +700,9 @@ Array is empty: 'tags':
 Array is empty: 'tags':
 ./config.json

+Missing file:
+./exercises/practice/bracket-push/.docs/instructions.md
+
 Array is empty: 'files.solution':
 ./exercises/concept/booleans/.meta/config.json

@@ -665,6 +714,16 @@ Array is empty: 'files.test':



+#### r
+Missing file:
+./exercises/practice/fizz-buzz/.docs/instructions.md

@@ -718,6 +777,37 @@ Missing key: 'tags':
 Array is empty: 'tags':
 ./config.json
+
+Missing file:
+./exercises/practice/microwave/.docs/instructions.md
+
+
+
+#### rust
+Missing file:
+./exercises/practice/decimal/.docs/instructions.md
+
+Missing file:
+./exercises/practice/doubly-linked-list/.docs/instructions.md
+
+Missing file:
+./exercises/practice/fizzy/.docs/instructions.md
+
+Missing file:
+./exercises/practice/luhn-from/.docs/instructions.md
+
+Missing file:
+./exercises/practice/luhn-trait/.docs/instructions.md
+
+Missing file:
+./exercises/practice/macros/.docs/instructions.md
+
+Missing file:
+./exercises/practice/xorcism/.docs/instructions.md


@@ -780,6 +870,9 @@ String is zero-length: 'blurb':

 Array is empty: 'tags':
 ./config.json
+
+Missing file:
+./exercises/practice/token-transfer/.docs/instructions.md


@@ -791,6 +884,9 @@ Array is empty: 'tags':
 Array is empty: 'tags':
 ./config.json

+Missing file:
+./exercises/practice/bracket-push/.docs/instructions.md
+
 Missing file:
 ./concepts/capturing/introduction.md

ee7
ee7 reviewed Mar 17, 2021
View reviewed changes
Copy link
Member

@ee7 ee7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out that ./exercises/practice does not exist for exactly these tracks:

- 05ab1e
- ada
- forth
- io
- nix
- research_experiment_1
- system-verilog

But the current state of the PR doesn't print an error message for a missing practice exercise directory, even though it would make configlet lint exit with a non-zero exit code.

src/lint/practice_exercises.nim Show resolved Hide resolved
@ee7 ee7 force-pushed the validate-practice-exercises branch from d6b4863 to 1069101 Compare March 17, 2021 13:18
@ee7 ee7 mentioned this pull request Mar 17, 2021
Merged
ErikSchierboom and others added 3 commits March 17, 2021 14:42
@ErikSchierboom @ee7
lint(practice_exercises): check required files exist
442f58d 
Each practice exercise's directory should contain at least the following
two files:
- .docs/instructions.md
- .meta/config.json
@ErikSchierboom @ee7
lint(practice_exercises): check .meta/config.json files
94f8f0f 
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
@ErikSchierboom @ee7
lint(practice_exercises): disable authors and files checks
699710a 
To prevent `configlet lint` from producing a lot of output for nearly
every track, we temporarily disable two checks.

With this commit, `configlet lint` will not check:
- The `files` property in the .meta/config.json file of each practice
  exercise.
- The `authors` property in the .meta/config.json file of each practice
  exercise.
@ee7 ee7 force-pushed the validate-practice-exercises branch from 1069101 to 699710a Compare March 17, 2021 13:43
ee7
ee7 approved these changes Mar 17, 2021
View reviewed changes
Copy link
Member

@ee7 ee7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved for "rebase and merge".

@ErikSchierboom ErikSchierboom merged commit 500e4a7 into main Mar 17, 2021
@ErikSchierboom ErikSchierboom deleted the validate-practice-exercises branch March 17, 2021 13:57
@ee7 ee7 changed the title lint(validators): validate practice exercises lint: add initial linting of practice exercises Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ee7 ee7 ee7 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ErikSchierboom @ee7