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.

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

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

Closed
verdammelt opened this issue Jan 22, 2022 · 9 comments · Fixed by #501
Closed

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

verdammelt opened this issue Jan 22, 2022 · 9 comments · Fixed by #501
Labels
cmd: sync kind: bug User-facing incorrect behavior

Comments

@verdammelt
Copy link
Member

verdammelt commented Jan 22, 2022

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
Copy link
Member

@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
Copy link
Member Author

verdammelt commented Jan 23, 2022

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
Copy link
Member

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

@verdammelt
Copy link
Member Author

@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
Copy link
Member

ee7 commented Jan 23, 2022

@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
Copy link
Member Author

verdammelt commented Jan 23, 2022

@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
Copy link
Member

ee7 commented Jan 25, 2022

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 #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:

  • The changes from configlet fmt
  • Adding configlet fmt to CI

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
Copy link
Member

ErikSchierboom commented Jan 25, 2022

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
Copy link
Member

ee7 commented Jan 25, 2022

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.

@ee7 ee7 added cmd: sync kind: bug User-facing incorrect behavior labels Jan 26, 2022
@ee7 ee7 closed this as completed in #501 Jan 27, 2022
ee7 added a commit that referenced this issue Jan 27, 2022
Before this commit, when updating a `.meta/config.json` file that
contained an optional key with the value of `null`, configlet would
produce an `UnpackDefect`. For example, the below tries to update a
file that contains `"contributors": null`:

    $ git clone https://github.com/exercism/common-lisp
    $ cd common-lisp
    $ git checkout 4fc524e24a69
    $ configlet sync --metadata -e binary -u
    Checking exercises...
    Cloning https://github.com/exercism/problem-specifications/... success
    [warn] metadata: unsynced: binary
    sync the above metadata ([y]es/[n]o)? y
    options.nim(194)         get
    Error: unhandled exception: Can't obtain a value from a `none` [UnpackDefect]

`configlet fmt` removes optional key/value pairs when the value is
`null`, and only the Common Lisp and Julia tracks had such a value, but
`configlet sync -u` should still handle this situation properly.

The bug: our `pretty` proc serializes from a list of keys, and relied on
optional values being non-null. `configlet fmt` added only keys with
non-null values to that list, but `configlet sync` tried to preserve
null values to reduce noise in diffs.

Fix by ensuring that `pretty` in sync mode can no longer sometimes use
`get` with a `none`.

Fixes: #500
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd: sync kind: bug User-facing incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants