Add help statement for cli #460

yageek · 2020-10-21T10:43:54Z

Refers to #372

coveralls · 2020-10-21T14:54:46Z

Pull Request Test Coverage Report for Build 917

0 of 72 (0.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.4%) to 55.205%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
internal/cli/main.go	0	72	0.0%

Totals
Change from base Build 901:	-0.4%
Covered Lines:	3044
Relevant Lines:	5514

💛 - Coveralls

dhui

Thanks for the PR!

dhui · 2020-10-23T22:02:21Z

internal/cli/main.go

@@ -145,6 +163,11 @@ Database drivers: `+strings.Join(database.List(), ", ")+"\n")
 		}

 	case "goto":
+		if flag.Arg(1) == helpArgument {


Use a flag.NewFlagSet() to parse the the argument. Can you create create a new method that creates a FlagSet with -help argument parsing and use that for all of the sub-commands?

I updated the PR

dhui

Thanks for addressing my feedback! Some of the code is repetitive and can be refactored.

dhui · 2020-10-28T04:00:44Z

internal/cli/main.go

 		if err := createFlagSet.Parse(args); err != nil {
 			log.Println(err)
 		}

+		if *help {
+			log.Println(createUsage)


Use fmt.Fprintln(os.Stderr, ...) for help messages so they're not modified when -verbose is specified.

dhui · 2020-10-28T04:05:03Z

internal/cli/main.go

 		if err := createFlagSet.Parse(args); err != nil {
 			log.Println(err)
 		}

+		if *help {


Factor this out into a common function: func handleSubCmdHelp(help bool, usage string, flagSet flag.FlagSet)

dhui · 2020-10-28T04:07:44Z

internal/cli/main.go

@@ -145,11 +176,25 @@ Database drivers: `+strings.Join(database.List(), ", ")+"\n")
 		}

 	case "goto":
+
+		gotoSet := flag.NewFlagSet("goto", flag.ExitOnError)


Factor this out into a common function that creates a new FlagSet with a help flag: func newFlagSetWithHelp(name string, errHandling flag.ErrorHandling) (*FlagSet, bool)

dhui

Thanks again for the PR and for addressing my feedback!

dhui

Fix linter issues

dhui · 2020-10-28T10:34:15Z

internal/cli/main.go

+
+func handleSubCmdHelp(help bool, usage string, flagSet *flag.FlagSet) {
+	if help {
+		fmt.Fprintln(os.Stderr, createUsage)


createUsage => usage

dhui · 2020-10-28T10:35:45Z

internal/cli/main.go

+		os.Exit(0)
+	}
+}
+func newFlagSetWithHelp(name string, errHandling flag.ErrorHandling) (*flag.FlagSet, *bool) {


Looks like the errHandling param is unused.

Might as well return bool instead of *bool.

I guess I have to return a pointer to a bool otherwise the value will always be the one at the creation and I will not get updated after flag has finished to parse the arguement.

Remove the errHandling param since it's always being passed the same value, which triggers the unparam linter:

internal/cli/main.go:41:38: `newFlagSetWithHelp` - `errHandling` always receives `flag.ExitOnError` (`1`) (unparam) func newFlagSetWithHelp(name string, errHandling flag.ErrorHandling) (*flag.FlagSet, *bool) { ^

dhui

Thanks for the fixes. Noticed one more issue now that the diff is much smaller.

dhui · 2020-10-31T03:12:54Z

internal/cli/main.go

 		if err := downFlagSet.Parse(args); err != nil {
-			log.fatalErr(err)
+			log.Println(err)


We should be consistent about how parse failures are handled in sub-commands. Previously, both the down and drop sub-commands would log.fatalErr so let's continue doing that.

dhui

Other flagset parsing that need to be updated to log.fatalErr

dhui · 2020-11-02T04:20:46Z

internal/cli/main.go

 		extPtr := createFlagSet.String("ext", "", "File extension")
 		dirPtr := createFlagSet.String("dir", "", "Directory to place file in (default: current working directory)")
 		formatPtr := createFlagSet.String("format", defaultTimeFormat, `The Go time format string to use. If the string "unix" or "unixNano" is specified, then the seconds or nanoseconds since January 1, 1970 UTC respectively will be used. Caution, due to the behavior of time.Time.Format(), invalid format strings will not error`)
 		createFlagSet.BoolVar(&seq, "seq", seq, "Use sequential numbers instead of timestamps (default: false)")
 		createFlagSet.IntVar(&seqDigits, "digits", seqDigits, "The number of digits to use in sequences (default: 6)")
+
 		if err := createFlagSet.Parse(args); err != nil {
 			log.Println(err)


log.fatalErr

dhui · 2020-11-02T04:20:53Z

internal/cli/main.go

+		gotoSet, helpPtr := newFlagSetWithHelp("goto")
+
+		if err := gotoSet.Parse(args); err != nil {
+			log.Println(err)


log.fatalErr

dhui · 2020-11-02T04:21:01Z

internal/cli/main.go

+		upSet, helpPtr := newFlagSetWithHelp("up")
+
+		if err := upSet.Parse(args); err != nil {
+			log.Println(err)


log.fatalErr

dhui · 2020-11-02T04:21:21Z

internal/cli/main.go

+		forceSet, helpPtr := newFlagSetWithHelp("force")
+
+		if err := forceSet.Parse(args); err != nil {
+			log.Println(err)


log.fatalErr

dhui

Thanks again for the PR and for addressing all of the feedback!

MarcvanMelle · 2020-11-04T14:50:38Z

internal/cli/main.go

-		if flag.Arg(1) != "" {
+		if flag.NArg() > 0 {


This breaks the CLI using migrate up (sans limit argument) because flag.NArg() is always > 0 because flag.Arg(0) is always up

yageek added 2 commits October 21, 2020 12:43

Add help statement for cli

ea82dcb

Refers to #372

Make constant for argument

0c4c7d2

dhui requested changes Oct 23, 2020

View reviewed changes

Create a flagset for each command

491ccb6

dhui requested changes Oct 28, 2020

View reviewed changes

yageek added 2 commits October 28, 2020 09:18

Factor error handling

c74fe52

Factor flagset creation

9fff514

dhui approved these changes Oct 28, 2020

View reviewed changes

dhui requested changes Oct 28, 2020

View reviewed changes

yageek added 2 commits October 28, 2020 12:23

fix: Typo and bad behaviour

fe81e39

Fix lint error

b3f52b7

dhui requested changes Oct 31, 2020

View reviewed changes

Fix according to the comments

42eeba4

dhui requested changes Nov 2, 2020

View reviewed changes

Fix according to the comments

f9f8e3b

dhui approved these changes Nov 3, 2020

View reviewed changes

dhui merged commit 8a56273 into golang-migrate:master Nov 3, 2020

MarcvanMelle reviewed Nov 4, 2020

View reviewed changes

renovate bot mentioned this pull request May 22, 2021

fix(deps): update module github.com/golang-migrate/migrate to v4 - autoclosed advenjourney/advenjourney#20

Closed

1 task

lunfel mentioned this pull request Oct 2, 2024

Update migrate -help output for the readme file #1179

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add help statement for cli #460

Add help statement for cli #460

yageek commented Oct 21, 2020

coveralls commented Oct 21, 2020 •

edited

Loading

dhui left a comment

dhui Oct 23, 2020

yageek Oct 26, 2020

dhui left a comment

dhui Oct 28, 2020

dhui Oct 28, 2020

dhui Oct 28, 2020

dhui left a comment

dhui left a comment

dhui Oct 28, 2020

dhui Oct 28, 2020

yageek Oct 28, 2020

dhui Oct 29, 2020

dhui left a comment

dhui Oct 31, 2020

dhui left a comment

dhui Nov 2, 2020

dhui Nov 2, 2020

dhui Nov 2, 2020

dhui Nov 2, 2020

dhui left a comment

MarcvanMelle Nov 4, 2020

dhui Nov 5, 2020

Add help statement for cli #460

Add help statement for cli #460

Conversation

yageek commented Oct 21, 2020

coveralls commented Oct 21, 2020 • edited Loading

Pull Request Test Coverage Report for Build 917

💛 - Coveralls

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhui left a comment

Choose a reason for hiding this comment

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 21, 2020 •

edited

Loading