Skip to content

Conversation

andreimerlescu
Copy link
Owner

In order to add WithAlias to the figtree, I needed to refactor a major functionality of the package.

figs := figtree.Grow()
figs = figs.NewString("name", "", "Your name")
figs = figs.WithAlias("name", "n")
err := figs.Parse()
if err != nil {
   panic(err)
}

This underlying task of NewString needed an internal refactor such that...

flag.String("name", "", "Your name") 

Became...

value := &Value{
  Value: "",
  Mutagenesis: tString,
}
flag.Var(value, "name", "Your name")

And in order to do this, how the .String() and FigFlesh() methods accessed and retrieved underlying value needed to be moved from a basic map[string]figFruit into sync.Map in tandem with the map[string]*figFruit and the value is never actually stored inside the *figFruit struct. However, a new method called tree.from("name") will return the *Value from the flag.Var declarations for the property called name.

This refactor bumps to the project to v2.0.11 and fixes known issues with using Aliases in v2.0.10.

@Copilot Copilot AI review requested due to automatic review settings June 22, 2025 11:32
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 pull request refactors the internal handling of flag values in figtree by moving from directly embedding values in figFruit to using a dedicated sync.Map of Value objects and introduces a new fluent API including the WithAlias method. Key changes include:

  • Refactoring all New* API methods to return a fluent Plant interface.
  • Updating all accesses from fig.Flesh.Flesh to lookups in tree.values.
  • Adding the new WithAlias method and updating tests and documentation accordingly.

Reviewed Changes

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

File Description
validators.go Added logging of nil values during validation.
alias.go Introduced WithAlias with fluent interface and conflict handling.
Various test files Updated tests to use fluent API and FigFlesh instead of Fig.
conversions.go, others Refactored conversion functions and updated flag handling.
Comments suppressed due to low confidence (1)

alias.go:11

  • [nitpick] When an alias conflict occurs, the method silently returns the tree. It would be beneficial to explicitly document this behavior or consider if overwriting, warning, or error propagation is more appropriate.
	if _, exists := tree.aliases[alias]; exists {

val = v.Value
}
if val == nil {
log.Printf("val is nil for %s", name)
Copy link

Copilot AI Jun 22, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider removing or replacing the debug log statement with a more formal error handling mechanism if this condition is not expected to occur in production.

Suggested change
log.Printf("val is nil for %s", name)
return fmt.Errorf("validation failed: value is nil for %s", name)

Copilot uses AI. Check for mistakes.

Comment on lines +17 to +22
fmt.Println("failed to load -" + name + " value")
return tree
}
value, ok := ptr.(*Value)
if !ok {
fmt.Println("failed to cast -" + name + " value")
Copy link

Copilot AI Jun 22, 2025

Choose a reason for hiding this comment

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

[nitpick] Instead of using fmt.Println for error messages, consider using a logging framework (e.g., log.Printf) or propagating an error to ensure consistent error handling in the library.

Suggested change
fmt.Println("failed to load -" + name + " value")
return tree
}
value, ok := ptr.(*Value)
if !ok {
fmt.Println("failed to cast -" + name + " value")
log.Printf("failed to load value for name: %s", name)
return tree
}
value, ok := ptr.(*Value)
if !ok {
log.Printf("failed to cast value for name: %s", name)

Copilot uses AI. Check for mistakes.

@andreimerlescu
Copy link
Owner Author

There are 3 commented out tests that are not passing that need to pass asap, but I'll allow this as-is for v2.0.11.

@andreimerlescu andreimerlescu merged commit 9bfcfb9 into main Jun 23, 2025
12 checks passed
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.

1 participant