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

renderer: Don't panic for unknown NodeKind #107

Closed
wants to merge 1 commit into from
Closed

renderer: Don't panic for unknown NodeKind #107

wants to merge 1 commit into from

Conversation

abhinav
Copy link
Contributor

@abhinav abhinav commented Mar 6, 2020

When rendering a Node, Renderer looks up the NodeRenderer for that
node's kind in a slice. That will panic with an "index out of range"
error if a node renderer for that node kind has not been registered.

This change verifies that a node renderer is available for that node
kind before attempting to use it, and instead of panicking, returns an
informative error message.

When rendering a Node, Renderer looks up the NodeRenderer for that
node's kind in a slice. That will panic with an "index out of range"
error if a node renderer for that node kind has not been registered.

This change verifies that a node renderer is available for that node
kind before attempting to use it, and instead of panicking, returns an
informative error message.
@yuin
Copy link
Owner

yuin commented Mar 7, 2020

This kind of PR has been submitted before.

Works as intended. This is coding problem so it should panic.

@yuin yuin closed this Mar 7, 2020
@abhinav
Copy link
Contributor Author

abhinav commented Mar 7, 2020

I can see the argument for this being a panic. The error message can be hard to decipher, though.

panic: runtime error: index out of range [1] with length 1

It definitely stumped me for a minute, which is what lead to this PR. The issue on my end was installing an extension on the parser but not on the renderer, which was instantiated separately.

I can update the PR to throw a panic with a more helpful error message if that's preferable.

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.

None yet

2 participants