Update to pure V json2 module#78
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64108e85af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| module gui | ||
|
|
||
| import json | ||
| import json2 |
There was a problem hiding this comment.
This import (and the same new import in locale_bundle.v) breaks the pinned Windows smoke workflow I checked: .github/workflows/windows-native-smoke.yml sets V_VERSION: 0.5.1 and compiles gui tests/examples, but V 0.5.1 exposes this module as x.json2 rather than top-level json2 (top-level vlib/json2 exists only in newer V). In that workflow the compiler will fail module resolution before any tests run, so either import x.json2 or bump the pinned toolchain together with this change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
What would be the best way to approach this? I imagine x.json2 won't be around forever but should we use it here for now until its deprecated or just stick with json2. I'd suggest the latter given the turbulence of V currently its probably okay to just track the latest compiler version.
There was a problem hiding this comment.
I checked both V 0.5.1 and current V. The minimal compatible solution is to use import x.json2 in both files: V 0.5.1 provides that module directly, while current V keeps it as an alias to the top-level json2 module. The existing json2.decode[...] calls can remain unchanged.
Tracking only the latest V version is also a valid policy, but that would require explicitly updating or retiring the pinned V 0.5.1 smoke workflow.
| return error('invalid JSON: expected object') | ||
| } | ||
| bundle := json.decode(LocaleBundle, trimmed) or { return error('invalid JSON: ${err}') } | ||
| bundle := json2.decode[LocaleBundle](trimmed) or { return error('invalid JSON: ${err}') } |
There was a problem hiding this comment.
When locale or theme bundles come from a serializer that escapes object-member names, switching this call to json2 stops treating those valid JSON names as their unescaped spelling: json2 matches struct/map keys against the raw bytes, so input like {"str\u0069ngs":{"o\u006b":"OK"}} no longer populates strings['ok'] and falls back to the default text. The previous cJSON-backed json.decode handled these escapes, so bundles with escaped keys regress here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This seems like more of an issue with json2 rather than GUI
There was a problem hiding this comment.
You are right, the actual bug must be fixed in json2, not worked around in GUI. However, this PR should wait for that upstream fix because switching decoders currently introduces the regression in GUI. Once json2 is fixed, we should add a small GUI regression test and complete the migration.
Would you be willing to open an issue in the V repository for this json2 escaped-key decoding problem, based on the regression we identified in GUI?
There was a problem hiding this comment.
I've opened an issue based on this though I haven't personally confirmed the behaviour identified by codex yet. If I get time later I will.
There was a problem hiding this comment.
Fix merged in V, so this can be handled as needed.
|
@squidink7 Alex has now fixed and merged the escaped-key decoding issue in json2: vlang/v#27815 Please update V first. To finish this PR, the cleanest option is to track the latest V version, keep import json2, update the V 0.5.1 smoke workflow, add a small GUI regression test for escaped locale/theme keys, then resolve both Codex review threads. Thanks! :) |
3 participants
jsonis deprecated, replaced byjson2