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

Upgrade urfave/cli to handle Sources on Int and FloatFlags #59

Closed
wants to merge 8 commits into from

Conversation

zacharygraziano
Copy link
Contributor

@zacharygraziano zacharygraziano commented Nov 25, 2024

We have been using a workaround package that implements HideDefault to get around the limitation that existed in urfave/cli v3.0.0-alpha9 (urfave/cli#1925). However, our workaround did not support additional sources for the value of Int or Float flags, and it was thus not possible to supply values for rooms-per-process and a number of other variables via config file.

The limitation is fixed in the latest alpha, so this PR upgrades to the most recent version of the package. Because of this, we can remove the workaround package and also get the benefit of Sources working again for integer and float flags. However, there are some other breaking changes in the latest release that also needed to be accounted for.

Persistent & Local Flags

Flags were previously local by default and could be set as persistent using the Persistent field. In the most recent release, flags are persistent by default, there is no Persistent field, and making a flag local requires setting the Local field. See urfave/cli#976 for context.

Stateful Flags

The documented behavior of flags in the library is that they are stateful and should not be reused. However in the version of the library we were using this seemed to not be the case and we were able to make all of our flags vars and the tests mostly ran (see: urfave/cli#2007). In the most recent release the behavior matches the documented behavior, so to continue with our current testing strategy all flags need to be used only once. To accomplish this this PR changes all flags and commands to be constructed via factory functions, so each test gets its own instance of the flag and previous cases don't cause unexpected issues in test runs.

Due to this behavior and the special implementation of the help flag in the library, the tests for the "help" commands were removed.

Double specification of global flags

We previously specified global flags both on the root command and on individual commands using the subcommandFlags function. This likely didn't matter because the flags in both cases were the same object. However, with the other changes in this PR, these are no longer the same object, and this ended up being a problem for parsing. Luckily, recent changes to the library have allowed global flags to be parsed in any order (urfave/cli#1987) and the fix for this was to remove the subcommandFlags function altogether.

@zacharygraziano
Copy link
Contributor Author

Closing in favor of #62 to fix the bug. We will revisit the upgrade in the future.

@zacharygraziano zacharygraziano deleted the upgrade-urfave-cli branch December 3, 2024 17:27
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