Skip to content

use create_default_context from nu-cmd-lang#9454

Merged
sholderbach merged 3 commits intonushell:mainfrom
amtoine:fix/benchmark-call-to-create-default-context
Jun 17, 2023
Merged

use create_default_context from nu-cmd-lang#9454
sholderbach merged 3 commits intonushell:mainfrom
amtoine:fix/benchmark-call-to-create-default-context

Conversation

@amtoine
Copy link
Copy Markdown
Member

@amtoine amtoine commented Jun 16, 2023

a recent addition must have removed the create_default_context command from nu_command, which breaks the benchmarks 😮

before this PR

cargo check --all-targets --workspace

gives a bunch of

error[E0425]: cannot find function `create_default_context` in crate `nu_command`
  --> benches/benchmarks.rs:93:48
   |
93 |             let mut engine_state = nu_command::create_default_context();
   |                                                ^^^^^^^^^^^^^^^^^^^^^^ not found in `nu_command`
   |
help: consider importing this function
   |
1  | use nu_cmd_lang::create_default_context;
   |
help: if you import `create_default_context`, refer to it directly
   |
93 -             let mut engine_state = nu_command::create_default_context();
93 +             let mut engine_state = create_default_context();
   |

and cargo bench does not run...

with this PR

cargo check --all-targets --workspace

is not happy and the benchmarks run again with cargo bench

@sholderbach
Copy link
Copy Markdown
Member

sholderbach commented Jun 16, 2023

This kind of changes the benchmark as this a much smaller set of commands in the engine state. nu_command::add_shell_command_context() will add the rest back in. See

nushell/crates/nu-cli/tests/support/completions_helpers.rs

Lines 13 to 15 in 86f12ff

fn create_default_context() -> EngineState {
nu_command::add_shell_command_context(nu_cmd_lang::create_default_context())
}
as an example how I hack the behavior consistent.

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Jun 16, 2023

@sholderbach
do you mean i should add

 fn create_default_context() -> EngineState { 
     nu_command::add_shell_command_context(nu_cmd_lang::create_default_context()) 
 }

inside the benchmark as well? 😋

@sholderbach
Copy link
Copy Markdown
Member

sholderbach commented Jun 16, 2023

That would be the codeduplication driven escape-hatch to some exquisite feature-flag-f***ery one could do to conditionally make it available from nu-command :P

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Jun 16, 2023

mm yeah but the current changes of the PR look easier, right? 😮

@sholderbach
Use both nu-cmd-lang and nu-commands
e94fc43 
Add a helper to create the command context. Reflects a realistic number
of entries in the engine state.
@sholderbach
fixup: cargo fmt
475d1f5
@sholderbach sholderbach merged commit 7d301c7 into nushell:main Jun 17, 2023
@amtoine amtoine deleted the fix/benchmark-call-to-create-default-context branch June 17, 2023 13:47
@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Jun 17, 2023

oooh i see @sholderbach, thanks for the patch 😌

amtoine added a commit to amtoine/nushell that referenced this pull request Jun 21, 2023
@amtoine
Merge commit '7d301c76d' into refactor/move-common-tools-from-nu-comm…
45db6b7 
…ands-to-nu-cmd-base

this should solve the `benches/` issue fixed in nushell#9454
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@amtoine @sholderbach