fix(schema): preserve doc option in nested {:map, opts} fields#532
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68591a827d
ℹ️ 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".
lib/mix/tasks/test.livebooks.ex
Outdated
| result = | ||
| try do | ||
| {output, exit_code} = | ||
| System.cmd("elixir", [temp_file], stderr_to_stdout: true, timeout: 30_000) |
There was a problem hiding this comment.
Remove unsupported :timeout option from System.cmd
The new livebook test task crashes as soon as it tries to execute a code block because System.cmd/3 does not accept a :timeout option (on the project’s supported Elixir versions, including ~> 1.15). This raises ArgumentError before any pass/fail handling runs, so mix test.livebooks cannot validate even a single notebook.
Useful? React with 👍 / 👎.
The option was being lost for nested map properties because was called with instead of the property options. This fix passes to preserve all options including which becomes the field in JSON Schema. Fixes agentjido#521
68591a8 to
4c3b8d0
Compare
Summary
Fixes #521 - The
doc:option was being lost for nested{:map, opts}fields inSchema.to_json/1.Problem
nimble_type_to_json_schema/2at line 460 was passing[]instead ofprop_optswhen converting nested map properties. This droppeddoc:(and any other opts) from nested properties — they never got a"description"field in the resulting JSON Schema.Fix
Changed
[]toprop_optsat line 460:Test
Added test verifying that nested map properties preserve their
doc:options and generate correct"description"fields in JSON Schema.Checklist
mix test)mix format)mix credo --strict)mix dialyzer)