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

Inconsistent Name for Application from inside Subcommands (v1) #973

Closed
wants to merge 12 commits into from

Conversation

asahasrabuddhe
Copy link
Member

@asahasrabuddhe asahasrabuddhe commented Dec 4, 2019

Motivation

Fixes #783

The motivation for doing this for v1 is to provide support, and a valuable feature that the users want to stick to v1 can appreciate.

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.
  • command.go - Added logic to initialise and update the ProgramName and CommandName members.
  • command-test.go - Added a test case to validate above change

Testing

I used the following program to help test:

package main

import (
	"fmt"
	"log"
	"os"

	"github.com/urfave/cli"
)

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()
	}
}

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.

@asahasrabuddhe asahasrabuddhe requested a review from a team as a code owner December 4, 2019 16:24
add err to the log
@codecov
Copy link

codecov bot commented Dec 4, 2019

Codecov Report

Merging #973 into v1 will increase coverage by 0.19%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v1     #973      +/-   ##
==========================================
+ Coverage   71.65%   71.85%   +0.19%     
==========================================
  Files          30       30              
  Lines        2410     2416       +6     
==========================================
+ Hits         1727     1736       +9     
+ Misses        577      574       -3     
  Partials      106      106
Impacted Files Coverage Δ
app.go 85.89% <100%> (+1.37%) ⬆️
command.go 84.39% <100%> (+0.36%) ⬆️

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 ebf9c3e...4d8a76d. Read the comment docs.

@asahasrabuddhe
Copy link
Member Author

This is still WIP

@coilysiren
Copy link
Member

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

Copy link
Member

@coilysiren coilysiren left a comment

Choose a reason for hiding this comment

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

(clearing the review request, per last comment)

@coilysiren
Copy link
Member

coilysiren commented Dec 8, 2019

Is this still relevant as of #975?

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

@coilysiren coilysiren changed the title Inconsistent Name for Application from inside Subcommands [v1] Inconsistent Name for Application from inside Subcommands Dec 18, 2019
@coilysiren
Copy link
Member

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

@stale
Copy link

stale bot commented Apr 17, 2020

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 stale bot added the status/stale stale due to the age of it's last update label Apr 17, 2020
@stale
Copy link

stale bot commented May 18, 2020

Closing this as it has become stale.

@stale stale bot closed this May 18, 2020
@meatballhat meatballhat reopened this Apr 19, 2022
@meatballhat meatballhat added area/v1 relates to / is being considered for v1 and removed status/stale stale due to the age of it's last update labels Apr 21, 2022
@meatballhat meatballhat changed the title [v1] Inconsistent Name for Application from inside Subcommands Inconsistent Name for Application from inside Subcommands (v1) Apr 21, 2022
@meatballhat meatballhat added this to the Release 1.22.x milestone Apr 21, 2022
that either don't work on v1 anymore or are specific to the `master`
branch.
@meatballhat meatballhat self-assigned this Apr 21, 2022
as 1.15.x is no longer supported by the Go team and both 1.17.x and
1.18.x can't run the tests
@meatballhat
Copy link
Member

In the end, I think I'd like to close this without merging so that the v1 branch can remain strictly for patches (1.22.x). I'll be opening a separate PR with only the CI config changes.

@meatballhat meatballhat deleted the inconsistent-program-name-v1 branch April 21, 2022 18:26
meatballhat added a commit that referenced this pull request Apr 21, 2022
meatballhat added a commit that referenced this pull request Apr 21, 2022
* add ProgramName and CommandName variables

* add comments to variables

* add test case

* refer program name instead of app name

* Update command_test.go

add err to the log

* fix failing tests

* Cut out many CI things

that either don't work on v1 anymore or are specific to the `master`
branch.

* More general v1 branch CI updates :-/

* More ~fighting~ playing with CI bits

* Drop back to only testing go 1.16.x

as 1.15.x is no longer supported by the Go team and both 1.17.x and
1.18.x can't run the tests

* Back out some changes from #973

Co-authored-by: Ajitem Sahasrabuddhe <ajitem.s@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v1 relates to / is being considered for v1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants