Skip to content

chore(deps): update github.com/urfave/cli to 3.8.0#2726

Open
cuixq wants to merge 6 commits intogoogle:mainfrom
cuixq:cli
Open

chore(deps): update github.com/urfave/cli to 3.8.0#2726
cuixq wants to merge 6 commits intogoogle:mainfrom
cuixq:cli

Conversation

@cuixq
Copy link
Copy Markdown
Contributor

@cuixq cuixq commented Apr 16, 2026

Unblocks #2674

For github.com/urfave/cli/v3 version 3.8.0, it seems an empty flag is no longer treated as an error but instead an empty flag will be set, see urfave/cli#2297.

Thus this PR updated the expected exit codes and error messages for test cases involving empty flags.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.72%. Comparing base (8ddcc85) to head (8a31a0b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2726      +/-   ##
==========================================
- Coverage   79.77%   79.72%   -0.05%     
==========================================
  Files         117      117              
  Lines        8016     8016              
==========================================
- Hits         6395     6391       -4     
- Misses       1246     1248       +2     
- Partials      375      377       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cuixq cuixq marked this pull request as ready for review April 16, 2026 02:02
@cuixq cuixq requested a review from another-rex April 16, 2026 02:02
tests := []testcmd.Case{
{
Name: "empty_plugins_flag_does_nothing",
Name: "empty_plugins_flag_does_default",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think this is correct - the value being empty should not cause it to fallback to the default

what I would expect to happen is plugins ends up with an empty string, which we should consider an error

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There's a separate test with --no-default that does what you describe, and stays that way.

@G-Rath
Copy link
Copy Markdown
Collaborator

G-Rath commented Apr 20, 2026

relating to my inline comment, could you check if this changes what happens with --config=? if it means we're using the default instead of erroring (one way or another), then frankly I'd say the upstream is wrong in their behaviour because it feels dangerous to fallback to the default.

overall this change has some weird potential implications, so I think we should dig a bit deeper to confirm what the actual result is

@another-rex
Copy link
Copy Markdown
Collaborator

I'm not sure that's what happens though? An empty plugins string with out experimental-no-default-plugins should just use the default plugins right?

@another-rex
Copy link
Copy Markdown
Collaborator

Pushed similar changes to #2674 directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants