refactor(menu)!: require Menu::settings instead of panicking in a default - #1215
Merged
kronberger-droid merged 3 commits intoSep 17, 2026
Merged
kronberger-droid merged 3 commits into
kronberger-droid merged 3 commits into
Conversation
The default body existed so that external menus written before `MenuSettings` kept compiling, and then panicked on first use, since every other default method on the trait reads through it. Nothing outside reedline implements `Menu`, so the compile error is the cheaper failure.
kronberger-droid
marked this pull request as ready for review
September 16, 2026 12:44
kronberger-droid
added a commit
that referenced
this pull request
Sep 17, 2026
…eam (#1220) #1215 made `Menu::settings` required, but its return type lives in the private `menu` module and was never re-exported. The panicking default had hidden that: an external impl could simply leave the method out. Without the export no crate outside reedline can implement `Menu`, which broke nushell's `SourcedMenu` wrapper on the bump to e0f1c0b.
kronberger-droid
added a commit
to nushell/nushell
that referenced
this pull request
Sep 17, 2026
## Description Fixes the broken build on `main` after #19042. nushell/reedline#1215 made `Menu::settings` a required method, and `SourcedMenu` never forwarded it, so `nu-cli` stopped compiling. This adds the forward and bumps reedline to c9e7035, which includes nushell/reedline#1220 exporting `MenuSettings`, since without that export the return type could not be named here. ## User-facing changes (Release notes) ## Additional notes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Menu::settingsships a default body that panics, added so menus written beforeMenuSettingskept compiling.The default
nameandindicatorread throughsettings(), and the engine looks a menu up bynameon every menu event, so a menu that takes the default compiles and then panics the first time it is triggered.Making the method required turns that guaranteed runtime panic into a compile error.
Breaking for
Menuimplementors outside reedline.Every in-tree menu already implements it, and nushell has no
Menuimpls of its own.Before
After
Additional notes
The second commit gives the painter's
TestMenua realMenuSettingsinstead of anunimplemented!(), since the point is to stop menus carrying a panic where their settings should be.