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

Simplified flag value access #1367

Merged

Conversation

toaster
Copy link
Contributor

@toaster toaster commented Apr 22, 2022

What type of PR is this?

  • feature

What this PR does / why we need it:

This PR implements convenience accessors to access flag values in a context.
Instead of writing ctx.Int(f.Name) or ctx.Value(f.Name).(int) one can write f.Get(ctx).
Thus, you don’t have to use the correct type method or the appropriate type assertion. The flag picks the correct name and gives you the correct type.

Which issue(s) this PR fixes:

Fixes #1316

Special notes for your reviewer:

I’m happy to give the accessor a shorter name if one comes up with a good idea :). (done! ~ @meatballhat)

Release Notes

All built-in flags now have a convenience method to access a flag’s value within a `Context`.
With `f.Get(ctx)` you don’t have to provide the flag’s name or its value’s actual type.

@toaster toaster requested a review from a team as a code owner April 22, 2022 15:21
@toaster toaster changed the title Feature/1316 simplified flag value access Feature: simplified flag value access Apr 22, 2022
@meatballhat meatballhat added kind/feature describes a code enhancement / feature request area/v2 relates to / is being considered for v2 labels Apr 22, 2022
@meatballhat meatballhat added this to the Release 2.5.0 milestone Apr 22, 2022
@meatballhat
Copy link
Member

meatballhat commented Apr 22, 2022

@toaster What do you think of claiming Get? This would be similarly as terse as flag.Getter.Get and future (v3+) versions could similarly define an interface method with return signature of any once we're beyond supporting go < 1.18.

@toaster
Copy link
Contributor Author

toaster commented Apr 25, 2022

@toaster What do you think of claiming Get? This would be similarly as terse as flag.Getter.Get and future (v3+) versions could similarly define an interface method with return signature of any once we're beyond supporting go < 1.18.

I like the idea and added a respective commit.
However, I’m not deep enough into the project to assess whether this fits the API.

@meatballhat meatballhat changed the title Feature: simplified flag value access Simplified flag value access Apr 26, 2022
Copy link
Member

@meatballhat meatballhat left a comment

Choose a reason for hiding this comment

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

I like it! I'll wait a bit before merging to make sure no other @urfave/cli humans have issues with the increased API surface area 👍🏼

@meatballhat meatballhat requested a review from a team April 26, 2022 12:14
@meatballhat meatballhat removed this from the Release 2.6.0 milestone Apr 28, 2022
@meatballhat meatballhat added this to the Release 2.x milestone Apr 28, 2022
@meatballhat
Copy link
Member

@dearchap when you get time, I'd appreciate your thoughts on this 🙇🏼

@dearchap
Copy link
Contributor

dearchap commented May 1, 2022

@meatballhat I have no problem with the new interface. This API is not within most people's workflow wherein they get the flag value from context but if users wants to use flag.GetFromContext thats fine with me.

@meatballhat meatballhat merged commit cbd9bd9 into urfave:main May 1, 2022
@xoxys
Copy link

xoxys commented May 20, 2022

Is there a minimal example on how to use it? On a minimal example from the docs, how could the new approach be used to access the value of the lang flag from the example below?

app := &cli.App{
    Flags: []cli.Flag{
      &cli.StringFlag{
        Name:    "lang",
        Aliases: []string{"l"},
        Value:   "english",
        Usage:   "language for the greeting",
        EnvVars: []string{"LEGACY_COMPAT_LANG", "APP_LANG", "LANG"},
      },
    },
  }

@meatballhat
Copy link
Member

@xoxys You will need a reference to the flag in order to use this type of access. For example, to take your code and put it into an example app to show the typical way of using *cli.Context, but with a modification demonstrating the source of bugs that's alleviated via direct flag access:

package main

import (
    "log"
    "os"

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

func main() {
    app := &cli.App{
        Flags: []cli.Flag{
            &cli.StringFlag{
                Name:    "lang",
                Aliases: []string{"l"},
                Value:   "english",
                Usage:   "language for the greeting",
                EnvVars: []string{"LEGACY_COMPAT_LANG", "APP_LANG", "LANG"},
            },
        },
        Action: func(cCtx *cli.Context) error {
            notLang := cCtx.String("1ang")
            log.Println("is this lang?:", notLang)

            lang := cCtx.String("lang")
            log.Println("is this lang?:", lang)

            return nil
        },
    }

    if err := app.Run(os.Args); err != nil {
        log.Fatal(err)
    }
}

and then modify it to use the access provided via the flag's Get method:

package main

import (
    "log"
    "os"

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

func main() {
    langFlag := &cli.StringFlag{
        Name:    "lang",
        Aliases: []string{"l"},
        Value:   "english",
        Usage:   "language for the greeting",
        EnvVars: []string{"LEGACY_COMPAT_LANG", "APP_LANG", "LANG"},
    }

    app := &cli.App{
        Flags: []cli.Flag{langFlag},
        Action: func(cCtx *cli.Context) error {
            notLang := langFlag.Get(cCtx)
            log.Println("is this lang?:", notLang)

            lang := langFlag.Get(cCtx)
            log.Println("is this lang?:", lang)

            return nil
        },
    }

    if err := app.Run(os.Args); err != nil {
        log.Fatal(err)
    }
}

Here's the diff, if helpful:

--- a/1367.go
+++ b/1367.go
@@ -8,21 +8,21 @@
 )

 func main() {
+       langFlag := &cli.StringFlag{
+               Name:    "lang",
+               Aliases: []string{"l"},
+               Value:   "english",
+               Usage:   "language for the greeting",
+               EnvVars: []string{"LEGACY_COMPAT_LANG", "APP_LANG", "LANG"},
+       }
+
        app := &cli.App{
-               Flags: []cli.Flag{
-                       &cli.StringFlag{
-                               Name:    "lang",
-                               Aliases: []string{"l"},
-                               Value:   "english",
-                               Usage:   "language for the greeting",
-                               EnvVars: []string{"LEGACY_COMPAT_LANG", "APP_LANG", "LANG"},
-                       },
-               },
+               Flags: []cli.Flag{langFlag},
                Action: func(cCtx *cli.Context) error {
-                       notLang := cCtx.String("1ang")
+                       notLang := langFlag.Get(cCtx)
                        log.Println("is this lang?:", notLang)

-                       lang := cCtx.String("lang")
+                       lang := langFlag.Get(cCtx)
                        log.Println("is this lang?:", lang)

                        return nil

I hope this is helpful 😁

@xoxys
Copy link

xoxys commented May 21, 2022

I hope this is helpful grin

That helps, thanks a lot!

@meatballhat meatballhat mentioned this pull request Jun 18, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2 kind/feature describes a code enhancement / feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify accessing flag values
4 participants