[v2] Inconsistent Name for Application from inside Subcommands #974

asahasrabuddhe · 2019-12-04T17:56:52Z

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.


        add ProgramName and CommandName variables


        add comments

refer to program name over app name


        add test case


        fix failing tests


        cleanup

AudriusButkevicius · 2019-12-04T19:49:02Z

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.


        Merge branch 'master' into inconsistent-program-name

codecov · 2019-12-05T06:54:37Z

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%)`	⬆️
command.go	`83.21% <100%> (+0.5%)`	⬆️

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 · 2019-12-05T07:00:38Z

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.

lynncyrin · 2019-12-06T20:32:11Z

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.

lynncyrin · 2019-12-06T20:39:56Z

So until someone can provide evidence

Also, this should be happening inside the issue => #783 rather than inside this pull request. Pull requests aren't a good context for collecting information and discussion like this.

lynncyrin · 2019-12-06T20:43:39Z

I consider this PR WIP and not approve-able until consensus on the desired solution is reached, that conversation should happen here => #783

lynncyrin · 2019-12-08T08:15:27Z

~~Similarly to #973, is this still relevant as of #975?~~

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

lynncyrin · 2020-01-18T22:16:18Z

For the sake of making this the most recent comment - this PR is blocked in discussion in => #783

stale · 2020-04-17T23:34:19Z

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 · 2020-05-18T00:10:43Z

Closing this as it has become stale.

asahasrabuddhe added 4 commits Dec 4, 2019

add ProgramName and CommandName variables

Loading status checks…

a1d37e2

add comments

996cebc

refer to program name over app name

add test case

Loading status checks…

5521c5a

fix failing tests

Loading status checks…

95d5772

asahasrabuddhe requested a review from urfave/cli as a code owner Dec 4, 2019

asahasrabuddhe changed the title ~~Inconsistent Name for Application from inside Subcommands~~ [v2] Inconsistent Name for Application from inside Subcommands Dec 4, 2019

cleanup

Loading status checks…

56dae23

Merge branch 'master' into inconsistent-program-name

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

7d6aa2d

lynncyrin mentioned this pull request Dec 6, 2019

v2 feature: Add an `App.ProgramName` #783

Closed

lynncyrin removed the request for review from urfave/cli Dec 6, 2019

lynncyrin mentioned this pull request Dec 6, 2019

[v1] Inconsistent Name for Application from inside Subcommands #973

Closed

lynncyrin mentioned this pull request Dec 8, 2019

Recieve and handle ctrl + c and exit application #953

Closed

lynncyrin assigned asahasrabuddhe Dec 8, 2019

stale bot added the status/stale label Apr 17, 2020

stale bot closed this May 18, 2020

urfave / cli

[v2] Inconsistent Name for Application from inside Subcommands #974

[v2] Inconsistent Name for Application from inside Subcommands #974

asahasrabuddhe commented Dec 4, 2019 •

edited

AudriusButkevicius commented Dec 4, 2019 •

edited

codecov bot commented Dec 5, 2019 •

edited

asahasrabuddhe commented Dec 5, 2019

lynncyrin commented Dec 6, 2019

lynncyrin commented Dec 6, 2019

lynncyrin commented Dec 6, 2019

lynncyrin commented Dec 8, 2019 •

edited

lynncyrin commented Jan 18, 2020

stale bot commented Apr 17, 2020

stale bot commented May 18, 2020

urfave / cli

Join GitHub today

[v2] Inconsistent Name for Application from inside Subcommands #974

[v2] Inconsistent Name for Application from inside Subcommands #974

Conversation

asahasrabuddhe commented Dec 4, 2019 • edited

Motivation

Release Notes

Changes

Testing

Reviewer Guidelines

AudriusButkevicius commented Dec 4, 2019 • edited

codecov bot commented Dec 5, 2019 • edited

Codecov Report

asahasrabuddhe commented Dec 5, 2019

lynncyrin commented Dec 6, 2019

lynncyrin commented Dec 6, 2019

lynncyrin commented Dec 6, 2019

lynncyrin commented Dec 8, 2019 • edited

lynncyrin commented Jan 18, 2020

stale bot commented Apr 17, 2020

stale bot commented May 18, 2020

asahasrabuddhe commented Dec 4, 2019 •

edited

AudriusButkevicius commented Dec 4, 2019 •

edited

codecov bot commented Dec 5, 2019 •

edited

lynncyrin commented Dec 8, 2019 •

edited