exercism / configlet

The official tool for managing Exercism language track repositories.
https://exercism.org/docs/building/configlet
GNU Affero General Public License v3.0
21 stars 14 forks source link

sync throws error when config.json has `"contributors": null` #500

Closed verdammelt closed 2 years ago

verdammelt commented 2 years ago

Not sure if this is an issue with configlet per se or a general problem of my track's practice execise config.json files. (UPDATE: this might be a lint bug since lint is reporting no problems and my config may in fact be invalid.)

Out of 43 exercises my track has 15 which has "contributors": null in their config.json files. (I don't think I did that myself so perhaps it is a left-over of some earlier tooling?)

At least one of these needs their metadata updated (for example 'binary') and I get the following when i try to update it:

./bin/configlet sync --update --yes --metadata -e binary
Checking exercises...
Cloning https://github.com/exercism/problem-specifications/... success
[warn] metadata: unsynced: binary
options.nim(194)         get
Error: unhandled exception: Can't obtain a value from a `none` [UnpackDefect]

Replacing the contributors value with [] or removing it altogether corrects the issue.

BethanyG commented 2 years ago

@verdammelt -- I think there was a change in the config.json spec a while ago that required either no key at all -- or a [] (array) for the contributors key. But configlet wasn't enforcing it. Now, it's enforced.

verdammelt commented 2 years ago

If by 'enforced' you mean: "sync will throw an error" then yep - seeing that behavior. If you mean "lint will signal a problem" then nope...

Quoting configlet lint output:

- Every practice exercise has a valid .meta/config.json file

BethanyG commented 2 years ago

oof! Yep, that looks like a problem...

verdammelt commented 2 years ago

@BethanyG thanks for the info. Sounds like this might actually be a lint issue...

I'm going to push a commit that fixes all my configs... I'll leave this issue so correct fix can be discussed

ee7 commented 2 years ago

@verdammelt Please run configlet fmt -uy as a workaround - this will format the exercise config files, removing the "contributors": null in the process.

I can confirm that the common-lisp track is the only track right now with this problem with contributors:

$ rg --sort path --files-with-matches -uu '"contributors": null'
common-lisp/exercises/practice/all-your-base/.meta/config.json
common-lisp/exercises/practice/atbash-cipher/.meta/config.json
common-lisp/exercises/practice/binary/.meta/config.json
common-lisp/exercises/practice/binary-search/.meta/config.json
common-lisp/exercises/practice/collatz-conjecture/.meta/config.json
common-lisp/exercises/practice/crypto-square/.meta/config.json
common-lisp/exercises/practice/grains/.meta/config.json
common-lisp/exercises/practice/hamming/.meta/config.json
common-lisp/exercises/practice/isogram/.meta/config.json
common-lisp/exercises/practice/leap/.meta/config.json
common-lisp/exercises/practice/pascals-triangle/.meta/config.json
common-lisp/exercises/practice/perfect-numbers/.meta/config.json
common-lisp/exercises/practice/prime-factors/.meta/config.json
common-lisp/exercises/practice/rna-transcription/.meta/config.json
common-lisp/exercises/practice/trinary/.meta/config.json
verdammelt commented 2 years ago

@ee7 will do.

I can confirm that the common-lisp track is the only track right now with this problem

Common Lisp - always at the front, leading the way! :/

ee7 commented 2 years ago

I should say: I think the problem is in configlet - not the track. It's good to have an issue to track this problem, which was known to me - see the to-do list at the end of the top post in https://github.com/exercism/configlet/pull/366:

do we want to have an actual JSON parsing error for things like "contributors": null, or e.g. silently replace it on updating?

(I'll eventually create issues for the other items).

I think the intention was to PR to every track with both:

which would avoid the issue by forcing tracks to have formatted config files... but we didn't get around to that yet.

But regardless, I agree that configlet lint and configlet sync should match - if configlet lint exits successfully, then configlet sync shouldn't produce a JSON parsing error. The current situation is partly because configlet lint and configlet sync each use a mix of two JSON parsing libraries - the library we use for syncing exercise config files is more ergonomic and performant, but there are cases where it cannot parse strictly enough to be the exclusive library for lint (and it didn't exist when we started implementing lint).

So I think the options are:

  1. Make configlet lint produce an error for null as the value of an optional key.
  2. Stop sync from producing an error for null as the value of an optional key.

If there aren't objections, I suggest we do option 2 because:

  1. "contributors": null is valid JSON - it's just "not formatted as configlet fmt formats"
  2. The website should correctly handle null as the value of an optional key

Agreed, @ErikSchierboom?

ErikSchierboom commented 2 years ago

Agreed. Option 2 is the best option I feel.

edit: p.s. the website will gracefully handle null values I believe (see https://github.com/exercism/website/blob/main/app/commands/git/sync_exercise_contributors.rb#L29) and the following snippet to show what to_a does:

> nil.to_a
 => [] 
ee7 commented 2 years ago

I wrote:

this problem, which was known to me

But it turns out that I misremembered. I'd already tried to do option 2, which is why the problem occurs only when updating (not when just syncing).

Fix in #501.