search

mattwparas / steel

An embedded scheme interpreter in Rust
Apache License 2.0
1.05k stars 49 forks source link

Improve error message for symbol missing from `BuiltInModuleRepr` #152

Closed wmedrano closed 6 months ago

wmedrano commented 6 months ago

Also slightly simplify maybe_get_environment_variable.

mattwparas commented 6 months ago

This LGTM - any objection to the fact that it panics? I think in this path the idea was fail fast early and loudly, but I could see an argument for not doing that

wmedrano commented 6 months ago

This LGTM - any objection to the fact that it panics? I think in this path the idea was fail fast early and loudly, but I could see an argument for not doing that

Maybe. I don't think I have enough experience to make an informed opinion yet. At the moment the panic only slightly annoyed me in the repl since the following caused a crash which exits the repl:

(#%require-dylib "libmylib" (only-in typo))
mattwparas commented 6 months ago

Works for me, if you become increasingly annoyed we can adjust it to not panic. Should be an easy change and we can follow up with it if necessary, doesn't block this by any means