Skip to content

Comments

feat!: add more integers and unsigned integers#2094

Merged
dearchap merged 2 commits intourfave:mainfrom
somebadcode:generic-int-flags
Apr 19, 2025
Merged

feat!: add more integers and unsigned integers#2094
dearchap merged 2 commits intourfave:mainfrom
somebadcode:generic-int-flags

Conversation

@somebadcode
Copy link
Contributor

  • Reintroduced all variants of integers and unsigned integers.

Fixes: #2050

What type of PR is this?

  • feature

What this PR does / why we need it:

Only having flags for int64 and uint64 when generics can be used to make it support all of them is very limiting and forces the users of the package to create wrappers if the variable that they want to configure using a flag is not part of their project or adds unnecessary bloat by having less appropriate integer types and conversions of those integers.

Which issue(s) this PR fixes:

Fixes #2050

Release Notes

(REQUIRED)

Added support for all integer types, signed and unsigned.

@dearchap
Copy link
Contributor

@somebadcode Can you run make v3approve and push your changes. That should fix the failing checks.

@somebadcode somebadcode force-pushed the generic-int-flags branch 2 times, most recently from 57dcf48 to c11b9b2 Compare April 15, 2025 21:48
@somebadcode somebadcode marked this pull request as ready for review April 15, 2025 21:53
@somebadcode somebadcode requested a review from a team as a code owner April 15, 2025 21:53
@somebadcode
Copy link
Contributor Author

@dearchap I've addressed the things you pointed out and I've added basic tests for all the new flag types.

@xoxys
Copy link

xoxys commented Apr 16, 2025

This change is breaking again, right? I'm wondering about how this will be released. Will we see v4 soon?

@somebadcode
Copy link
Contributor Author

This change is breaking again, right? I'm wondering about how this will be released. Will we see v4 soon?

Yes, it's breaking since it changes Int from int64 to int.
So that's what I'm wondering. The same thing has to be done with floats. Not supporting all the builtin types was a huge mistake. The flag package should be the minimum requirement. Any package that try to be an alternative should have the same base feature-set.

I was a bit too slow to make it to the v3 release. I initially hoped that they'd held back the release of v3 because of these mistakes since the use of generics makes is really easy to cover all the different integers and floats.

So maybe correcting these short-comings will cause a v4 in a not too distant future?

@dearchap
Copy link
Contributor

Yes breaking change. Can't help it.

@somebadcode somebadcode force-pushed the generic-int-flags branch 4 times, most recently from 5c53cb1 to ff934ef Compare April 17, 2025 18:36
@dearchap
Copy link
Contributor

@somebadcode Final request. There are 2 spots not covered by any tests. Not sure why

flag_uint.go#L55-L59 and L61 .

Basically the String() function of uintValue[T]. Not sure why its not covered since we have Uint.String() tests in flag_test.go(see TestUintFlagXXX functions). Can you fix that ?

* Reintroduced all variants of integers and unsigned integers.
* Fixed the use of base by using `IntegerConfig.Base` and defaulting to base 10 when formatting.

Fixes: urfave#2050
Signed-off-by: Tobias Dahlberg <lokskada@live.se>
* `extFlag.PreParse()` will set the value to that of `flag.DefValue`.
@dearchap
Copy link
Contributor

Awesome @somebadcode . Thank you so much.

@dearchap dearchap merged commit e8c32ad into urfave:main Apr 19, 2025
10 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.

Removal of integer flags causes issues.

3 participants