Inconsistent Name for Application from inside Subcommands

[asahasrabuddhe]

Motivation

Fixes #783

The current behavior of the app.Name is not consistent and results in a bad experience for the users. The PR attempts to fix this until such time a better solution is in place (v3)

Release Notes

Adds ProgramName and CommandName. The ProgramName will always contain the name of the application as defined by the user. The CommandName will include the ProgramName with the name of the command, and it's parent commands.

Changes

• app.go - Added the ProgramName and CommandName members to the App struct. Initialize ProgramName for the first time.
• command.go - Added logic to initialise and update the ProgramName and CommandName members. Make use of the context lineage to set the ProgramName correctly.
• command-test.go - Added a test case to validate above change'
• helpers_test.go - Added equal method to compare slices

Testing

I used the following program to help test:

package main

import (
	"fmt"
	"log"
	"os"

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

func main() {
	app := cli.NewApp()
	app.Name = "myprogramname"
	app.Action = func(c *cli.Context) error {
		fmt.Println("c.App.Name for app.Action is", c.App.Name)
		fmt.Println("c.App.ProgramName for app.Action is", c.App.ProgramName)
		fmt.Println("c.App.CommandName for app.Action is", c.App.CommandName)
		return nil
	}
	app.Commands = []*cli.Command{
		{
			Name: "foo",
			Action: func(c *cli.Context) error {
				fmt.Println("c.App.Name for app.Commands.Action is", c.App.Name)
				fmt.Println("c.App.ProgramName for app.Commands.Action is", c.App.ProgramName)
				fmt.Println("c.App.CommandName for app.Commands.Action is", c.App.CommandName)
				return nil
			},
			Subcommands: []*cli.Command{
				{
					Name: "bar",
					Action: func(c *cli.Context) error {
						fmt.Println("c.App.Name for App.Commands.Subcommands.Action is", c.App.Name)
						fmt.Println("c.App.ProgramName for App.Commands.Subcommands.Action is", c.App.ProgramName)
						fmt.Println("c.App.CommandName for App.Commands.Subcommands.Action is", c.App.CommandName)
						return nil
					},
					Subcommands: []*cli.Command{
						{
							Name: "baz",
							Action: func(c *cli.Context) error {
								fmt.Println("c.App.Name for App.Commands.Subcommands.Subcommands.Action is", c.App.Name)
								fmt.Println("c.App.ProgramName for App.Commands.Subcommands.Subcommands.Action is", c.App.ProgramName)
								fmt.Println("c.App.CommandName for App.Commands.Subcommands.Subcommands.Action is", c.App.CommandName)
								return nil
							},
						},
					},
				},
			},
		},
	}

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

The same program was modified to write the test case.

Reviewer Guidelines

The PR is marked as a v2 feature. However, I thought that this, being a simple change, it would be worth our while to get this into v1 for the sake of sanity.

[AudriusButkevicius]

I don't think we should add any public API, we should just fix App.Name to always be the same thing. I think command can be retrieved from the context, or if the user is generating functions (the only reason why I can explain you'd need a name), he could curry (functionally speaking) the name into the acting function.

[codecov[bot]]

Codecov Report

Merging #974 into master will increase coverage by 0.18%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #974      +/-   ##
==========================================
+ Coverage   73.36%   73.54%   +0.18%     
==========================================
  Files          32       32              
  Lines        2440     2446       +6     
==========================================
+ Hits         1790     1799       +9     
+ Misses        540      537       -3     
  Partials      110      110

Impacted Files	Coverage Δ
app.go	78.34% <100%> (+1.36%)	:arrow_up:
command.go	83.21% <100%> (+0.5%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 72c7fac...7d6aa2d. Read the comment docs.

[asahasrabuddhe]

I don't think we should add any public API, we should just fix App.Name to always be the same thing. I think command can be retrieved from the context, or if the user is generating functions (the only reason why I can explain you'd need a name), he could curry (functionally speaking) the name into the acting function.

@lynncyrin ☝️

@AudriusButkevicius You are right about currying through the context. We actually have a ctx.Lineage call available which would return a slice of contexts from the current till the parent which would help the user do the same.

[coilysiren]

I don't think we should add any public API, we should just fix App.Name to always be the same thing.

It's unclear if anyone relies on the current behavior of App.Name. So until someone can provide evidence that App.Name was always intended to ever return the program name, I consider this change to be a feature request rather than a fix.

[coilysiren]

So until someone can provide evidence

Also, this should be happening inside the issue => https://github.com/urfave/cli/issues/783 rather than inside this pull request. Pull requests aren't a good context for collecting information and discussion like this.

[coilysiren]

I consider this PR WIP and not approve-able until consensus on the desired solution is reached, that conversation should happen here => https://github.com/urfave/cli/issues/783

[coilysiren]

~Similarly to https://github.com/urfave/cli/pull/973, is this still relevant as of https://github.com/urfave/cli/pull/975?~

EDIT: nevermind this! I got my issues mixed up.

[coilysiren]

For the sake of making this the most recent comment - this PR is blocked in discussion in => https://github.com/urfave/cli/issues/783

[stale[bot]]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[stale[bot]]

Closing this as it has become stale.

[asahasrabuddhe]

@meatballhat i see there’s a test failing. Will take a look at this later today.

[dearchap]

@asahasrabuddhe Can you check if we still need this in latest release ? I've made some changes that actually fixes this.

[dearchap]

Fixed in latest release.