Conversation
Contributor
Reviewer's GuideThis PR introduces a set of into helper functions for configuration, environment, and theme handling, refactors existing callers to use these out-parameter helpers instead of subshell command substitutions, and splits var_default_value/varname_to_appname/config logic into reusable *_into implementations while preserving existing behavior. Flow diagram for config_get_into and backend selectionflowchart TD
Caller["Caller (e.g. env_sanitize, config_show)"] --> CG["config_get"]
CG --> CGI["config_get_into(section_key, config_file)"]
CGI -->|"file_extension == toml"| CTGI["config_toml_get_into(section_key, config_file)"]
CGI -->|"file_extension == ini"| CIGI["config_ini_get_into(section_key, config_file)"]
CTGI --> GTVI["get_toml_val_into(file, section.key)"]
CTGI --> GTVSI["get_toml_val_string_into(...) / get_toml_val_bool_into(...)"]
CIGI --> GIVI["get_ini_val_into(file, key)"]
CIGI --> GIVSI["get_ini_val_string_into(...) / get_ini_val_bool_into(...)"]
GTVI --> OutVal["OutVar (config value)"]
GTVSI --> OutVal
GIVI --> OutVal
GIVSI --> OutVal
Flow diagram for var_default_value_into and callersflowchart TD
CallerMenu["Caller (env_sanitize, menu_value_prompt, etc.)"] --> VDV["var_default_value"]
VDV --> VDVIntoCall["var_default_value_into(out_var, VarName)"]
VDVIntoCall --> VTAI["varname_to_appname_into(APPNAME, VarName)"]
VDVIntoCall -->|"APP var"| AppCase["APP case"]
VDVIntoCall -->|"APPENV var"| AppEnvCase["APPENV case"]
VDVIntoCall -->|"no APPNAME"| GlobalCase["GLOBAL case"]
AppCase --> AIFApp["app_instance_file_into(DefaultAppVarFile, APPNAME, .env)"]
AIFApp --> EnvExists["env_var_exists(CleanVarName, DefaultAppVarFile)"]
EnvExists -->|"exists"| EGLIApp["env_get_literal_into(out_var, CleanVarName, DefaultAppVarFile)"]
EnvExists -->|"missing"| AppFallback["compute hardcoded defaults for APP vars"]
AppEnvCase --> AIFEnv["app_instance_file_into(DefaultAppVarFile, APPNAME, .env.app.*)"]
AIFEnv --> EnvExistsEnv["env_var_exists(CleanVarName, DefaultAppVarFile)"]
EnvExistsEnv -->|"exists"| EGLEnv["env_get_literal_into(out_var, CleanVarName, DefaultAppVarFile)"]
EnvExistsEnv -->|"missing"| AppEnvFallback["default to ''"]
GlobalCase --> GlobalSwitch["GLOBAL var switch"]
GlobalSwitch -->|"COMPOSE/CONFIG paths"| UseLiterals["use LITERAL_COMPOSE_FOLDER / LITERAL_CONFIG_FOLDER"]
GlobalSwitch -->|"PGID/PUID"| UseDetected["use DETECTED_PGID / DETECTED_PUID"]
GlobalSwitch -->|"fallback"| ComposeDefault["env_get_literal_into from COMPOSE_ENV_DEFAULT_FILE or ''"]
EGLIApp --> OutVal["OutVar (default value)"]
AppFallback --> OutVal
EGLEnv --> OutVal
AppEnvFallback --> OutVal
UseLiterals --> OutVal
UseDetected --> OutVal
ComposeDefault --> OutVal
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new *_get_into helpers (e.g., config_ini_get_into, config_toml_get_into, config_get_into) compute the file extension using
${var_##*.}, which is invalid parameter expansion and will not yield the intended extension; this should be corrected to${var##*.}. - theme_name_into always returns successfully and defaults to an empty string on failure (
|| true), which changes the behavior compared to the previous theme_name implementation; consider propagating non‑zero status or explicitly handling the "no theme configured" case so callers can distinguish between a missing theme and a valid empty value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new *_get_into helpers (e.g., config_ini_get_into, config_toml_get_into, config_get_into) compute the file extension using `${var_##*.}`, which is invalid parameter expansion and will not yield the intended extension; this should be corrected to `${var##*.}`.
- theme_name_into always returns successfully and defaults to an empty string on failure (`|| true`), which changes the behavior compared to the previous theme_name implementation; consider propagating non‑zero status or explicitly handling the "no theme configured" case so callers can distinguish between a missing theme and a valid empty value.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request
Purpose
Describe the problem or feature in addition to a link to the issues.
Approach
How does this change address the problem?
Open Questions and Pre-Merge TODOs
Check all boxes as they are completed
Learning
Describe the research stage
Links to blog posts, patterns, libraries or addons used to solve this problem
Requirements
Check all boxes as they are completed
Summary by Sourcery
Refactor configuration, environment, and helper scripts to introduce *_into variants that write results to variables and update existing callers to use them for improved composability and performance.
Enhancements:
Tests: