-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat: Lang::intern_symbols
#1181
Conversation
32a96e3
to
666fa41
Compare
* Add `Lang::intern_symbols` to facilitate lang installations * Use `Lang::intern_symbols` on the REPL * Create an alias for `Rc<RefCell<State>>` and use it * Rewrite some functions to one-liners
666fa41
to
d4341d4
Compare
pub fn intern_symbol( | ||
&mut self, | ||
symbol: &Symbol, | ||
create_unknown_packges: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo, should create_unknown_packages
.
pub fn intern_symbol( | ||
&mut self, | ||
symbol: &Symbol, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is okay, although it points to a weirdness — which is that symbols should usually be interned. So it's a bit funny to pass a symbol to a function meant to intern it. We should try to work toward a world where the default is for symbols to be interned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because of our API to create symbols from strings like ".lurk.foo"
out of thin air. It was removed on the mega pr but was reintroduced later on with the help of the symbol parser
pub fn mk_lang<F: LurkField>(state: &StateRcCell) -> Lang<F, TrieCoproc<F>> { | ||
let mut lang = Lang::new(); | ||
install(state, &mut lang); | ||
lang |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a weird public function. I would normally expect that one would want to build Lang
s with a number of different coprocessors, composed with something like install
. In a world where that's possible, which we target, a function like this would be little more than a convenience for testing, in most cases. What about putting this in the test module or something? I'm a little wary of adding it to the public API since that might create confusion about what is sensible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather see mk_lang
be in the test module, but this all seems basically fine.
Closing in favor of #1189, which moves us closer to where we want to be |
Lang::intern_symbols
to facilitate lang installationsLang::intern_symbols
on the REPLRc<RefCell<State>>
and use it