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

Replace keybinding name fromJust with maybe #511

Merged
merged 1 commit into from
Jun 30, 2024

Conversation

xsebek
Copy link
Contributor

@xsebek xsebek commented Jun 29, 2024

When I forgot a KeyEvents entry and tried to print all keybindings, the fromJust failed with an error that does not show the problematic handler.

Instead of erroring out, write (unnamed) into Event Name, so that it's easy to see which handler is missing a name.

@@ -124,7 +124,7 @@ mkKeybindEventHelp kc h =
ByKey b ->
(Comment "(non-customizable key)", [Verbatim $ ppBinding b])
ByEvent ev ->
let name = fromJust $ keyEventName (keyConfigEvents kc) ev
let name = fromMaybe "<unnamed>" $ keyEventName (keyConfigEvents kc) ev
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtdaugherty I don't understand why this failed for me - I do not see <unnamed> in the output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must have missed one event in KeyEvents and then fixed it before creating this PR.

I deleted one, and the text is shown now:

| Keybinding | Event Name | Description |
| ---------- | ---------- | ----------- |
| `C-q` | (unnamed) | Open quit game dialog |

@xsebek xsebek force-pushed the keybinding-name-from-just branch from 7c84b2c to 7015b40 Compare June 29, 2024 15:25
@xsebek xsebek changed the title Replace keybinding name fromJust with fromMaybe Replace keybinding name fromJust with maybe Jun 29, 2024
@jtdaugherty
Copy link
Owner

I'm a little confused - could you provide some context for this patch? The commit message does not provide a motivation for it, so I don't know what problem this is intended to solve.

@xsebek
Copy link
Contributor Author

xsebek commented Jun 30, 2024

@jtdaugherty I updated the description. 👍 Simply put I run into a fromJust error and I wished there was more information. All I needed is in the rest of the printed line, so I made the code safely handle the situation.

@jtdaugherty
Copy link
Owner

Oh, so the higher-level problem is that you had a key event in existence that was not in the KeyEvents mapping so it had no name, and the pretty-printer didn't handle that well.

@jtdaugherty jtdaugherty merged commit d09f31e into jtdaugherty:master Jun 30, 2024
@jtdaugherty
Copy link
Owner

Thanks for your patch!

@jtdaugherty
Copy link
Owner

This is now released in 2.3.2.

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.

2 participants