Fix #7760 Use nu_engine::get_config in core code paths#10483
Fix #7760 Use nu_engine::get_config in core code paths#10483jakeswenson wants to merge 3 commits intonushell:mainfrom
Conversation
| parts.push(eval_expression(engine_state, stack, expr)?); | ||
| } | ||
|
|
||
| // TODO: Should this move to nu_engine::env::get_config? |
There was a problem hiding this comment.
Actually, this is a good place to watch out for performance: Running string interpolation in a hot loop would likely be slower with cloning the config. It might be worthwhile changing the nu_engine::env::get_config to output a Cow, or something like that—that could be a separate PR.
There was a problem hiding this comment.
Cow<'a, T> comes with a lifetime and optimizes for potential writes at the call.
Sounds like a situation for the Arc<T> if it only needs reads (I assume we need Send/Sync for par-each?) or Arc<Mutex<T>> if we need a writeable config.
But yeah benchmarking is probably the first step with something like this.
There was a problem hiding this comment.
The problem is that nu_engine::env::get_config() can return either a reference, or an owned value, depending on whether it comes from the stack or the engine.
|
You should be able to test it by running something like We should also watch out for performance because now we're cloning the config every time. You could, for example, keep track of the startup time—if it rises, we could look into optimizing it. And finally, you don't need to convert every single instance of |
|
Closing this PR in favor of the changes in #10613 Thanks for all the discussion @kubouch and @sholderbach! I hope you do mind moving it to the new PR! |
Description
This PR fixes #7760 and updates the usage of
EngineState::get_configtonu_engine::get_configin some core places in nu. There are still many many more places that useget_config...User-Facing Changes
Tests + Formatting
Need advice on testing this. This was something that was very difficult to test since this seems to reproduce only when the config file is set. the test framework for the testbin
nu_replneeds a significant rewrite in order to make this testable, and doesn't actually test using the real REPL..After Submitting
N/A - just a bug fix
fixes #7760