Skip to content
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

Removal of integer flags causes issues. #2050

Open
3 tasks done
somebadcode opened this issue Feb 5, 2025 · 4 comments
Open
3 tasks done

Removal of integer flags causes issues. #2050

somebadcode opened this issue Feb 5, 2025 · 4 comments
Labels
area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this status/waiting-for-response Waiting for response from original requester

Comments

@somebadcode
Copy link

somebadcode commented Feb 5, 2025

Checklist

  • Are you running the latest v3 release? The list of releases is here.
  • Did you check the manual for your release? The v3 manual is here.
  • Did you perform a search about this feature? Here's the GitHub guide about searching.

What problem does this solve?

The new opinionated IntFlag does not document on how to work with other types than int64. If I have a struct which I want to configure using flags, I do not want to restrict myself to using int64 since the struct is not defined by my project. I also do not want to create custom flags for builtin types.

I've tried to use the Action function on the flag to set the value for a field of type int but it doesn't work with default values, making any workaround too convoluted.

The issue of intValue not being exported cause issues if I want to do any form of customization of the flag and this applies to all other flag types. Allowing reuse in custom flags would be nice.

Solution description

Use generics to implement IntFlag, Int8Flag, Int16Flag, Int32Flag and Int64Flag.

Describe alternatives you've considered

One way would be to recreate the types that you removed but use generics to avoid code duplication.

package clix

import (
	"strconv"
	"unsafe"

	"github.com/urfave/cli/v3"
)

type IntFlag = cli.FlagBase[int, IntegerConfig, *IntValue[int]]
type Int8Flag = cli.FlagBase[int8, IntegerConfig, *IntValue[int8]]
type Int16Flag = cli.FlagBase[int16, IntegerConfig, *IntValue[int16]]
type Int32Flag = cli.FlagBase[int32, IntegerConfig, *IntValue[int32]]
type Int64Flag = cli.FlagBase[int64, IntegerConfig, *IntValue[int64]]

// IntegerConfig is the configuration for all integer type flags.
type IntegerConfig struct {
	Base int
}

// Validate will ensure that the base is set. If base is 0 then it set to 10. This function is called by [IntValue.Create].
func (cfg *IntegerConfig) Validate() {
	if cfg.Base == 0 {
		cfg.Base = 10
	}
}

type IntValue[T int | int8 | int16 | int32 | int64] struct {
	Val  *T
	Base int
}

func (i *IntValue[T]) Create(val T, p *T, c IntegerConfig) cli.Value {
	c.Validate()

	*p = val

	return &IntValue[T]{
		Val:  p,
		Base: c.Base,
	}
}

func (i *IntValue[T]) ToString(b T) string {
	return strconv.FormatInt(int64(b), i.Base)
}

func (i *IntValue[T]) Set(s string) error {
	v, err := strconv.ParseInt(s, i.Base, int(unsafe.Sizeof(T(0))))
	if err != nil {
		return err
	}

	*i.Val = T(v)

	return err
}

func (i *IntValue[T]) Get() any { return *i.Val }

func (i *IntValue[T]) String() string { return strconv.FormatInt(int64(*i.Val), i.Base) }

All methods use pointer receivers as recommended by the Go documentation. https://go.dev/doc/faq#methods_on_values_or_pointers
Making all the receivers either values or pointers avoids mistakes. Linters and IDEs will pick up on this and warn you about mixing values and pointers.

I can contribute a solution based on this if so desired.

@somebadcode somebadcode added area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this labels Feb 5, 2025
@dearchap
Copy link
Contributor

dearchap commented Feb 8, 2025

@somebadcode yes please open a PR. I can review

@Skeeve
Copy link
Contributor

Skeeve commented Feb 10, 2025

@somebadcode: what about the unsigned variants?

@somebadcode
Copy link
Author

@somebadcode: what about the unsigned variants?

I would add that too.

@dearchap
Copy link
Contributor

@somebadcode See PR #1765 as well.

@dearchap dearchap added the status/waiting-for-response Waiting for response from original requester label Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this status/waiting-for-response Waiting for response from original requester
Projects
None yet
Development

No branches or pull requests

3 participants