Skip to content

Conversation

Equationzhao
Copy link
Owner

No description provided.

@Equationzhao Equationzhao marked this pull request as draft May 21, 2025 11:55
@Equationzhao Equationzhao added the enhancement New feature or request label May 21, 2025
@Equationzhao
Copy link
Owner Author

image
Using the same flag, the column order is inconsistent. This should be fixed.

@Equationzhao
Copy link
Owner Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request successfully upgrades the urfave/cli dependency from v2 to v3. The changes correctly address the breaking API changes introduced in v3, such as the new Action signatures, the transition from cli.App to cli.Command, and flag property changes like DisableDefaultText to HideDefault.

The migration appears to be thorough and well-executed across all affected files.

My only piece of feedback concerns the man page generation, which has been removed as urfave/cli/v3 no longer supports it directly. I've left a comment in man/man.go with a suggestion on how to address this functionality regression.

Comment on lines 3 to 5
func GenMan() {
// man
man, _ := os.Create(filepath.Join("man", "g.1.gz"))
s, _ := cli.G.ToMan()
// compress to gzip
manGz := gzip.NewWriter(man)
defer manGz.Close()
_, _ = manGz.Write([]byte(s))
_ = manGz.Flush()
}

Choose a reason for hiding this comment

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

medium

I see that the man page generation logic has been removed. This is understandable as urfave/cli/v3 has removed the ToMan() functionality.

This change introduces a functionality regression, as the man page will no longer be generated.

To restore this, you could consider using a third-party library for man page generation, as recommended by the urfave/cli authors. One such library is https://github.com/cpuguy83/go-md2man.

Alternatively, if this functionality is no longer desired, it would be good to remove the GenMan function and its call site in main.go to avoid confusion, as the function is now empty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant