Skip to content

refactor(aqua): store aqua var defaults as strings#9645

Merged
jdx merged 3 commits intojdx:mainfrom
risu729:refactor/aqua-var-default-string
May 7, 2026
Merged

refactor(aqua): store aqua var defaults as strings#9645
jdx merged 3 commits intojdx:mainfrom
risu729:refactor/aqua-var-default-string

Conversation

@risu729
Copy link
Copy Markdown
Contributor

@risu729 risu729 commented May 6, 2026

Summary

  • Store aqua var defaults as Option<String> instead of preserving arbitrary YAML values.
  • Rely on Serde's normal Option<String> YAML deserialization; there is no custom default validator or tagged-value handling.
  • Simplify runtime var resolution now that defaults are stored as strings.

Notes

  • Upstream aqua's schema/type allows arbitrary var default values.
  • mise intentionally stores defaults as strings for implementation simplicity.
  • With serde_yaml, scalar defaults deserialize into strings, explicit null deserializes as no default, and sequence/mapping defaults fail during typed YAML parsing.
  • The vendored registry currently only uses string defaults or required vars without defaults.

Test Plan

  • cargo fmt --package aqua-registry --check
  • cargo check -p aqua-registry
  • cargo test -p aqua-registry

This PR body was generated by an AI coding assistant.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 6, 2026

Greptile Summary

This PR refactors AquaVar.default from Option<serde_yaml::Value> to Option<String>, delegating scalar-to-string coercion to serde's native deserialization rather than a custom yaml_var_to_string helper. The var_value resolution shrinks to a simple clone, and two helper functions (yaml_var_to_string, yaml_value_kind) are removed.

  • The core type change is clean: Option<String> is a simpler contract, and the var_value function becomes trivially correct.
  • Tests are improved by exercising deserialization through the full RegistryYaml path rather than constructing AquaPackage directly with pre-built serde_yaml::Values.

Confidence Score: 5/5

The refactor is safe to merge — the core logic change is correct and the vendored registry only uses string defaults.

The switch from Option<serde_yaml::Value> to Option<String> is mechanically sound. All call sites of var_value are unchanged, var_value itself now just clones an already-typed Option<String>, and serde handles the deserialization contract. The vendored registry only contains string defaults, so no production YAML is affected.

crates/aqua-registry/src/types.rs — the new test_vars_scalar_defaults_deserialize_as_strings test is the only area worth a second look given serde_yaml 0.9's strict scalar typing.

Reviews (4): Last reviewed commit: "Merge branch 'main' into refactor/aqua-v..." | Re-trigger Greptile

Comment thread crates/aqua-registry/src/types.rs Outdated
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request transitions the AquaVar default field from a generic YAML value to a specific Option, simplifying variable resolution. The changes include a new validation step in the build process to ensure all defaults in the registry are strings, a custom deserializer for AquaVar, and updated test cases. The review feedback suggests that the aqua_var_default_from_yaml function's return type is misleading as it never returns None, recommending a change to Result<String, String> for better clarity.

Comment thread crates/aqua-registry/src/types.rs Outdated
@risu729
Copy link
Copy Markdown
Contributor Author

risu729 commented May 6, 2026

CI failure looks unrelated to this AquaVar change. windows-e2e failed in e2e-win/zig.Tests.ps1 because downloading https://pkg.hexops.org/zig/zig-windows-x86_64-0.14.0-dev.2577+271452d22.zip timed out after three attempts. test-ci then failed only because it aggregates the failed windows-e2e result.

This comment was generated by an AI coding assistant.

Comment thread crates/aqua-registry/src/types.rs
@risu729
Copy link
Copy Markdown
Contributor Author

risu729 commented May 7, 2026

Greptile noted that test_vars_scalar_defaults_deserialize_as_strings should fail because serde_yaml would not coerce YAML bool/number scalars to String. I checked this locally in this workspace: cargo test -p aqua-registry passes, including test_vars_scalar_defaults_deserialize_as_strings; serde_yaml 0.9 deserializes those scalar defaults into strings for the Option<String> field.

The build/codegen validator was intentionally removed per review direction. The PR body now says the actual behavior: scalar defaults deserialize into strings, explicit null becomes no default, and sequence/mapping defaults fail during typed YAML parsing.

This comment was generated by an AI coding assistant.

@risu729 risu729 force-pushed the refactor/aqua-var-default-string branch from 524e5d3 to 1e6a72a Compare May 7, 2026 02:14
@risu729 risu729 marked this pull request as ready for review May 7, 2026 02:28
@jdx jdx merged commit 009c95b into jdx:main May 7, 2026
34 checks passed
@risu729 risu729 deleted the refactor/aqua-var-default-string branch May 7, 2026 11:17
mise-en-dev added a commit that referenced this pull request May 7, 2026
### 🚀 Features

- **(aqua)** support registry libc variants by @jdx in
[#9652](#9652)
- **(bin-paths)** add executable names output by @risu729 in
[#9617](#9617)

### 🐛 Bug Fixes

- **(aqua)** preserve configured file extensions by @risu729 in
[#9611](#9611)
- **(aqua)** support registry file links by @risu729 in
[#9610](#9610)
- **(backend)** reject bare package backend names by @risu729 in
[#9608](#9608)
- **(backend)** apply inline tool option overrides by @risu729 in
[#9306](#9306)
- **(backend)** skip versions host for local tool opts by @risu729 in
[#9568](#9568)
- **(github)** chmod explicit archive bin by @risu729 in
[#9609](#9609)
- **(install)** skip remote-versions refresh in prefer-offline mode by
@jdx in [#9627](#9627)
- **(lock)** scope targets to active project root by @risu729 in
[#9319](#9319)
- **(lockfile)** respect existing platforms during auto-lock by @jdx in
[#9621](#9621)
- **(pipx)** filter yanked pypi releases by @risu729 in
[#9607](#9607)
- **(pipx)** declare python as a backend dependency by @jdx in
[#9678](#9678)
- **(schema)** update refs to $defs in mise-registry-tool.json by
@risu729 in [#9671](#9671)
- **(task)** terminate parallel siblings on failure via process groups
by @jdx in [#9655](#9655)
- **(task)** stable MISE_PROJECT_ROOT for monorepo tasks, add
MISE_MONOREPO_ROOT by @jdx in
[#9657](#9657)
- **(trust)** run enter hooks after trusting config by @risu729 in
[#9634](#9634)
- **(ui)** stop clearing screen for prompts by @jdx in
[#9619](#9619)
- use /bin/cp on macos by @pdehlke in
[#9656](#9656)

### 🚜 Refactor

- **(aqua)** store aqua var defaults as strings by @risu729 in
[#9645](#9645)
- **(config)** support structured TOML values in registry backend
options by @risu729 in [#9584](#9584)
- **(deps)** remove serde_derive dependency by @risu729 in
[#9670](#9670)
- **(deps)** remove anyhow dependency by @risu729 in
[#9661](#9661)
- **(deps)** use std::sync::LazyLock instead of once_cell::Lazy by
@risu729 in [#9668](#9668)
- **(schema)** generate task schema from mise schema by @risu729 in
[#9581](#9581)
- **(schema)** reuse task props with unevaluatedProperties by @risu729
in [#9582](#9582)
- **(schema)** reuse registry common types by @risu729 in
[#9648](#9648)
- **(schema)** reuse plugin script config by @risu729 in
[#9647](#9647)
- **(schema)** use $defs in schema files by @risu729 in
[#9646](#9646)

### 📚 Documentation

- **(node)** add tips for enabling node idiomatic by @fu050409 in
[#9675](#9675)

### 🧪 Testing

- **(cli)** remove nondeterministic tool depends assertion by @risu729
in [#9633](#9633)
- **(e2e)** pin uv to 0.11.8 around astral-sh/uv#19278 by @jdx in
[#9618](#9618)
- **(e2e)** wait for docker env cleanup by @risu729 in
[#9631](#9631)
- **(zig)** use official zig instead of mach mirror by @jdx in
[#9659](#9659)

### 📦️ Dependency Updates

- fall through to hash check when providers have no outputs by @jdx in
[#9622](#9622)
- bump Cargo.lock by @jdx in
[#9625](#9625)

### 📦 Registry

- remove registry depends by @risu729 in
[#9571](#9571)
- add code-review-graph (pipx:code-review-graph) by @chautruonglong in
[#9673](#9673)

### Chore

- **(ci)** split large registry test-tool changes by @risu729 in
[#9628](#9628)
- **(ci)** make perf script robust to runner noise by @jdx in
[#9635](#9635)
- **(ci)** skip hyperfine comments without permission by @risu729 in
[#9629](#9629)

### New Contributors

- @chautruonglong made their first contribution in
[#9673](#9673)
- @pdehlke made their first contribution in
[#9656](#9656)

## 📦 Aqua Registry Updates

### New Packages (5)

-
[`anthropics/anthropic-cli`](https://github.com/anthropics/anthropic-cli)
- [`crates.io/wasmi_cli`](https://github.com/wasmi-labs/wasmi)
- [`openclaw/gogcli`](https://github.com/openclaw/gogcli)
- `racket-lang.org/racket-minimal`
- [`runs-on/cli`](https://github.com/runs-on/cli)

### Updated Packages (13)

- [`UpCloudLtd/upcloud-cli`](https://github.com/UpCloudLtd/upcloud-cli)
- [`aristocratos/btop`](https://github.com/aristocratos/btop)
- [`dprint/dprint`](https://github.com/dprint/dprint)
- [`j178/prek`](https://github.com/j178/prek)
- [`jdx/hk`](https://github.com/jdx/hk)
- [`jdx/mise`](https://github.com/jdx/mise)
- [`jdx/usage`](https://github.com/jdx/usage)
- [`jreleaser/jreleaser`](https://github.com/jreleaser/jreleaser)
-
[`jreleaser/jreleaser/standalone`](https://github.com/jreleaser/jreleaser)
- [`pnpm/pnpm`](https://github.com/pnpm/pnpm)
- [`suzuki-shunsuke/cmdx`](https://github.com/suzuki-shunsuke/cmdx)
- [`suzuki-shunsuke/ghir`](https://github.com/suzuki-shunsuke/ghir)
- [`twpayne/chezmoi`](https://github.com/twpayne/chezmoi)
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