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

Merge stack before printing #9304

Merged
merged 7 commits into from May 30, 2023
Merged

Merge stack before printing #9304

merged 7 commits into from May 30, 2023

Conversation

zhiburt
Copy link
Contributor

@zhiburt zhiburt commented May 27, 2023

Could you @fdncred try it?

close?: #9264

Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com>
@fdncred
Copy link
Collaborator

fdncred commented May 27, 2023

@zhiburt Doesn't seem to work. Here's my test.

  1. save preview-theme.nu from some theme items do not change colors when requested #9264 (comment)
  2. source preview-theme.nu
  3. cd nu_scripts
  4. preview and i randomly picked apprentice
  5. which then puts this on the command line use themes/themes/apprentice.nu; $env.config.color_config = (apprentice); preview_theme | table -e and hit enter
  6. I get this
    image
    Which you can tell it's not working because the header and row index (which are the same color) {fg: #87af87, attr: b} are not being drawn in that color. the green header and cyan index are from my current theme, before i use'd the one above.

If i run the command in 5 above again, i get this
image

One last comment here. After the first screenshot is shown with the preview_theme | table -e, if I do a ls, the theme is applied, even though the table that is drawn right after doesn't show it.
image

@fdncred
Copy link
Collaborator

fdncred commented May 27, 2023

This is so strange. If I do the commands on separate lines, it seems to work.
image

@fdncred
Copy link
Collaborator

fdncred commented May 27, 2023

It seems this may have something to do with how nushell is handling one-liners with multiple commands separated with semicolons? like all the stuff that is changing in this pr may not be happening on this type of one-liner?

Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com>
Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com>
@zhiburt
Copy link
Contributor Author

zhiburt commented May 28, 2023

I think must be addressed.
I got lazy a bit here and just followed @kubouch suggestion for this case.

Let me know if it works.

@fdncred
Copy link
Collaborator

fdncred commented May 29, 2023

wow, this is working great! still playing but just wow!!!

@fdncred
Copy link
Collaborator

fdncred commented May 29, 2023

@zhiburt I'm ready to merge this but can we have more descriptive fn names for merge_env2 and merge_env3?

Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com>
zhiburt and others added 3 commits May 29, 2023 23:57
Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>
@fdncred
Copy link
Collaborator

fdncred commented May 30, 2023

Thanks @zhiburt!

@fdncred fdncred merged commit 7f758d3 into nushell:main May 30, 2023
16 checks passed
@kubouch
Copy link
Contributor

kubouch commented May 30, 2023

I didn't have time to look into it yet but we shouldn't clone the EngineState before every print, that will be super slow.

@zhiburt
Copy link
Contributor Author

zhiburt commented May 30, 2023

Precisely clone will make it slower.
The same for table.

I am not sure to which degree though; you could test it.

Ideally the env shall be just merged somewhere in nu-engine/eval; but I haven't cracked this one.

kubouch added a commit to kubouch/nushell that referenced this pull request Jun 4, 2023
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

3 participants