Skip to content

refactor(ubi): parse tool options locally#9873

Merged
jdx merged 2 commits into
jdx:mainfrom
risu729:refactor/ubi-typed-options
May 17, 2026
Merged

refactor(ubi): parse tool options locally#9873
jdx merged 2 commits into
jdx:mainfrom
risu729:refactor/ubi-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 UBI typed-parser follow-up.

Summary

  • Add a private UbiOptions wrapper for UBI backend option reads.
  • Move provider/API, matching, executable/layout, and lockfile option reads 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 wraps all UBI backend option reads in a new private UbiOptions<'a> struct that delegates to BackendOptions, replacing scattered opts.get(key) calls throughout the file and aligning with the typed-parser approach introduced in #9838.

  • UbiOptions centralises every option accessor (provider, api_url, bin_path, extract_all, exe, rename_exe, matching, matching_regex, lockfile_options), removing duplicated lookup/clone boilerplate from several call-sites.
  • The opts.clone() inside the try_with_v_prefix closure is correctly dropped because UbiOptions<'a> derives Copy.
  • The strip_suffix("/")strip_suffix('/') change is semantically equivalent in Rust.

Confidence Score: 5/5

Pure mechanical refactor — no new logic, no control-flow changes, safe to merge.

All changes are structural: call-site boilerplate is replaced by the new UbiOptions wrapper, and the only substantive difference is that is_truthy now also accepts "1" for extract_all, which widens (rather than breaks) existing behavior.

No files require special attention.

Important Files Changed

Filename Overview
src/backend/ubi.rs Refactors option reads into a private UbiOptions<'a> wrapper backed by BackendOptions; logic is preserved except extract_all now also accepts "1" via the shared is_truthy helper.

Reviews (3): Last reviewed commit: "fix(ubi): use string option reader for e..." | Re-trigger Greptile

Comment thread src/backend/ubi.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 structured approach to managing backend options by implementing a BackendOptions wrapper and specialized structs like GitBackendOptions and UbiOptions. These changes centralize the logic for accessing configuration values, including platform-specific overrides, and refactor the GitHub and Ubi backends to utilize these new structures. A critical issue was identified in the UbiOptions implementation where an incorrect method name was used to retrieve the extract_all option.

Comment thread src/backend/ubi.rs Outdated
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/ubi-typed-options branch from c6ee718 to 4a9859d Compare May 16, 2026 07:49
Comment thread src/backend/ubi.rs
@risu729 risu729 marked this pull request as ready for review May 16, 2026 08:23
@jdx jdx merged commit fb07f4d into jdx:main May 17, 2026
32 checks passed
@risu729 risu729 deleted the refactor/ubi-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