config, patched_stdlib: remove patched json and parsejson#818
Draft
ee7 wants to merge 1 commit intoexercism:mainfrom
Draft
config, patched_stdlib: remove patched json and parsejson#818ee7 wants to merge 1 commit intoexercism:mainfrom
ee7 wants to merge 1 commit intoexercism:mainfrom
Conversation
No longer produce an error for JSON that contains a comment or trailing comma.
ErikSchierboom
approved these changes
Sep 19, 2023
This file contains hidden or 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
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.
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 lintchecks, 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.nimandparsejson.nimisn't ideal either. I think the long-term options are:std/[json, parsejson], and a regular jsony. This is the status quo.I do think that Exercism shouldn't allow trailing commas - they're bad for portability. For example,
jqproduces 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
JsonNodecode 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: