Skip to content

refactor(aqua): parse tool options locally#9872

Merged
jdx merged 1 commit into
jdx:mainfrom
risu729:refactor/aqua-typed-options
May 17, 2026
Merged

refactor(aqua): parse tool options locally#9872
jdx merged 1 commit into
jdx:mainfrom
risu729:refactor/aqua-typed-options

Conversation

@risu729

@risu729 risu729 commented May 15, 2026

Copy link
Copy Markdown
Contributor

Depends on merged #9838. This branch has been rebased onto current main, so the PR diff is now only the Aqua typed-parser follow-up.

Summary

  • Add a private AquaOptions wrapper for Aqua backend option reads.
  • Move symlink_bins, Aqua vars, and lockfile option collection behind that wrapper.
  • Keep behavior unchanged; this only follows the typed-parser direction from refactor(backend): parse tool options per backend #9838.

Verification

  • cargo fmt --all -- --check
  • git diff --check upstream/main...HEAD

This PR was generated by an AI coding assistant.

@greptile-apps

greptile-apps Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR extracts Aqua-specific option parsing into a new AquaOptions<'a> wrapper struct, following the typed-options pattern introduced in the parent PR. No logic is changed — the three moved functions (symlink_bins, var, lockfile_options) are textually identical to the originals.

  • AquaOptions::symlink_bins replaces the old AquaBackend::symlink_bins method, delegating to the new BackendOptions::bool helper which behaves identically to the original get_string(...).is_some_and(|v| v == "true" || v == "1") (the only difference is is_truthy adds a .trim() call, which is strictly more lenient).
  • AquaOptions::var and AquaOptions::lockfile_options are verbatim lifts of the former free function aqua_var_option and the static method AquaBackend::lockfile_options.
  • apply_var_options now takes &AquaOptions<'_> instead of &ToolVersionOptions, and call sites are updated to construct an AquaOptions from the raw options before passing it in.

Confidence Score: 5/5

Pure structural refactor with no logic changes; all moved code is textually identical to the original, and the new bool helper in BackendOptions is a straightforward delegation.

Every moved function is a verbatim lift from its original location. The only net-new code is BackendOptions::bool, which is a one-liner composing two already-tested primitives (get_string + is_truthy). Test coverage tracks the refactored call sites and the existing test assertions remain unchanged.

No files require special attention.

Important Files Changed

Filename Overview
src/backend/aqua.rs Moves symlink_bins, var/aqua_var_option, and lockfile_options/insert_vars_lockfile_options from AquaBackend static methods and module-level functions into a new AquaOptions<'a> wrapper struct; logic is byte-for-byte identical to the original.
src/backend/options.rs Adds a simple bool helper to BackendOptions that mirrors the existing platform_bool_for_target logic but without platform resolution, used by AquaOptions::symlink_bins.

Reviews (2): Last reviewed commit: "refactor(aqua): parse tool options local..." | Re-trigger Greptile

Comment thread src/backend/github.rs
Comment thread src/backend/github.rs

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

Copy link
Copy Markdown
Contributor

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 introduces a centralized BackendOptions utility and backend-specific wrappers, AquaOptions and GitBackendOptions, to standardize the handling of tool configuration options. This refactoring enables consistent support for platform-specific overrides across different backends. Review feedback pointed out that the is_truthy helper is currently case-sensitive and suggested a more robust implementation using lowercase matching. Additionally, it was noted that the new implementation for no_app detection now correctly respects platform-specific settings, which represents a change in behavior from the previous implementation.

Comment thread src/backend/github.rs
Comment thread src/backend/options.rs
jdx pushed a commit that referenced this pull request May 15, 2026
## Summary
- Add a shared `BackendOptions` reader for raw and platform-scoped
backend tool options.
- Add a private typed option wrapper for the unified
GitHub/GitLab/Forgejo backend.
- Move GitHub/GitLab/Forgejo option reads and lockfile option collection
behind that wrapper while keeping common install/verify helpers on raw
`ToolVersionOptions`.
- Keep HTTP, S3, Aqua, and UBI migrations out of this PR; those are
split into separate draft follow-ups.
- Defer `Backend::option_specs()` metadata changes to a later PR.

## Project item
-
https://github.com/users/risu729/projects/3/views/1?pane=issue&itemId=185472417
- PR C from project item "Refactor and fix tool option persistence by
phase".
- PR A: #9742, merged.
- PR B: #9753, merged.
- Draft follow-ups currently split from the original PR C scope:
  - HTTP: #9870
  - S3: #9871
  - Aqua: #9872
  - UBI: #9873

## Behavior notes
- This PR is intended to be a refactor and does not change
lockfile/manifest persistence classifications.
- The refactor includes two small behavior fixes found while
centralizing Git backend option reads:
- Forgejo now shares the same git install-time option filtering as
GitHub/GitLab.
- `no_app` is now read through target-aware platform option lookup, so
`platforms.<target>.no_app` works for cross-platform lock resolution.

## Review
- Gemini and Greptile AI review suggestions were addressed and their
review threads are resolved.
- Manual review focused on preserving existing string/scalar coercion
and platform fallback behavior in the Git backend wrapper.

## Verification
- `cargo fmt --all -- --check`
- `git diff --check upstream/main...HEAD`
- GitHub CI is pending for `fda4e0227` after narrowing the PR scope.

*This PR was updated by an AI coding assistant.*
@risu729 risu729 force-pushed the refactor/aqua-typed-options branch from d5e274a to 9fbc918 Compare May 16, 2026 07:49
@risu729 risu729 marked this pull request as ready for review May 16, 2026 08:23
@jdx jdx merged commit b49e7de into jdx:main May 17, 2026
32 checks passed
@risu729 risu729 deleted the refactor/aqua-typed-options branch May 17, 2026 15:31
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