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

config, patched_stdlib: remove patched json and parsejson #818

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Sep 15, 2023

No longer produce an error for JSON that contains a comment or trailing comma.


I'd need to think about this, but my initial feeling is that I don't love this idea. Here's an argument against it:

If Exercism rejects JSON comments and trailing commas, I think it's best to catch that in CI. There are many other JSON things that configlet lint checks, which could instead be dropped and deferred until sync time. Why should trailing commas be the thing we don't check at CI time? That's not obvious to me.

It's a worse experience for the maintainer to create a PR, get it passing CI, merge it, but then have it fail to sync and need to create another PR. And I think trailing commas aren't uncommon.

However, maintaining a patched json.nim and parsejson.nim isn't ideal either. I think the long-term options are:

  1. Exercism forbids both comments and trailing commas at CI time. Configlet uses patched std/[json, parsejson], and a regular jsony. This is the status quo.
  2. Exercism forbids both comments and trailing commas at CI time. Configlet uses a patched jsony, and works towards dropping the stdlib modules.
  3. Exercism forbids trailing commas at sync time only. Configlet uses a regular jsony (which forbids comments), drops the patched stdlib modules, and works towards dropping the stdlib modules.
  4. Exercism forbids both comments and trailing commas at sync time only. Configlet uses unpatched stdlib modules and unpatched jsony. This is the result of merging this PR.

I do think that Exercism shouldn't allow trailing commas - they're bad for portability. For example, jq produces an error for a trailing comma.

Perhaps merging this PR is only worthwhile when it means we can migrate fully to jsony. But there's a lot of JsonNode code to migrate before we can do that - the checks for JSON comments and trailing commas aren't the main blocker. And I think the burden of the stdlib modules in the codebase is not from the patches.

However, other advantages of this PR:

  • make it easier to migrate from nimble to atlas

No longer produce an error for JSON that contains a comment or trailing
comma.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants