这是indexloc提供的服务,不要输入任何密码
Skip to content

Fix: Support for nested pointer struct fields in CLI options #824

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

tomMoulard
Copy link
Contributor

@tomMoulard tomMoulard commented May 18, 2025

Description

This PR adds support for nested options in CLI configurations, particularly
when using pointer-to-struct fields. It addresses an issue where the CLI would
panic with panic: Unsupported option type: ptr or panic: Unsupported option type: struct
when attempting to set values on fields within a nil pointer struct.

Example Use Case (my use case).

I was trying to implement nested configuration options with pointers for a clean optional field pattern:

type DatabaseConfig struct {
    Host     string `doc:"Database host"`
    Port     int    `doc:"Database port" default:"5432"`
    Username string `doc:"Database username"`
}

type AppConfig struct {
    Debug bool            `doc:"Enable debug mode"`
    DB    *DatabaseConfig `doc:"Database configuration"` // Here both ptr or direct would have been acceptable.
}

When attempting to use this pattern with CLI flags like
--db.host localhost --db.port 5432,
I encountered a panic error: panic: Unsupported option type: struct.
The fix in this PR allows for properly handling these nested pointer-to-struct
configurations, so users can effectively organize related options.

Changes

  • Improved handling of nested fields by properly initializing nil pointers
    before setting values
  • Enhanced error handling with more descriptive messages and proper error
    wrapping

I tried to split every major change in its own commit, btw

Testing

Added a new test case TestCLINestedOptions that verifies both direct struct
fields and pointer-to-struct fields work correctly with CLI arguments and
environment variables.

Oh ! And thanks for your awesome project 💯

@Copilot Copilot AI review requested due to automatic review settings May 18, 2025 22:52
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a panic issue when using nested pointer-to-struct CLI options by initializing nil pointers before setting nested field values and enhancing error messages. It also adds tests for both pointer and direct nested struct configurations.

  • Supports nested pointer initialization in CLI options.
  • Improves error handling for parsing failures.
  • Adds dedicated tests for nested options via CLI flags and environment variables.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
humacli/humacli_test.go Adds tests verifying that nested pointer-to-struct and direct struct options handle inputs correctly.
humacli/humacli.go Introduces logic to properly initialize parent pointers and enhances error handling in setupOptions.

@tomMoulard tomMoulard marked this pull request as draft May 18, 2025 23:00
@tomMoulard
Copy link
Contributor Author

tomMoulard commented May 18, 2025

Sorry, I got bated by my bad test 😢

@tomMoulard tomMoulard force-pushed the feat/cli-recursive-structs branch from e01eead to 92bf536 Compare May 18, 2025 23:40
@tomMoulard tomMoulard marked this pull request as ready for review May 18, 2025 23:41
@tomMoulard tomMoulard requested a review from Copilot May 18, 2025 23:41
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes nested pointer-to-struct field support in CLI options by properly initializing nil pointers and handling field value extraction from flags and environment variables.

  • Initializes nil pointer fields before accessing nested fields.
  • Introduces helper functions to manage value precedence between CLI flags, environment variables, and defaults.
  • Adds unit tests to verify both nested pointer and direct struct option handling and priority between flags and environment variables.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
humacli/humacli_test.go Added tests to validate nested options and flag/env priority handling
humacli/humacli.go Updated option parsing, nil pointer initialization, and flag registration for nesting
Comments suppressed due to low confidence (1)

humacli/humacli.go:234

  • Undefined variable 'i' used when accessing a field; it appears the loop that iterates over 'opt.path' was removed. Reintroduce the loop to iterate over each index in 'opt.path' before accessing a field.
f = f.Field(i)

Copy link
Owner

@danielgtaylor danielgtaylor left a comment

Choose a reason for hiding this comment

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

Thank you!

@danielgtaylor danielgtaylor merged commit 60980ec into danielgtaylor:main Jun 19, 2025
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