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

Remove deprecated environment functionality #6468

Merged
merged 21 commits into from Sep 25, 2022

Conversation

WindSoilder
Copy link
Collaborator

@WindSoilder WindSoilder commented Sep 3, 2022

Description

while reading https://hackmd.io/@nucore/r1zilyw6q to see what's the future about module environment, I sopt that most of thing are done.

And export env doesn't work with source-env command.

So this pr is trying to help to remove export env command, and adjust relative tests and examples

About three more ignored tests:

  1. use_env_import_after_hide: it's more about hide-env, overlay, export-env commands
  2. hides_all_envs_within_scope: it's more about hide-env, overlay, export-env commands
  3. remove_overlay_keep_discard_overwritten_env: it's more about overlay with export-env issue itself, so I think it's ok to remove it

They are existing issue about the communication of overlay, export-env and hide-env commands, I think we can keep it ignored for now, and fix them in separate pr.
But I think it's also ok to wait these relative issue to be fixed firstly.

Tests

Make sure you've done the following:

  • Add tests that cover your changes, either in the command examples, the crate/tests folder, or in the /tests folder.
  • Try to think about corner cases and various ways how your changes could break. Cover them with tests.
  • If adding tests is not possible, please document in the PR body a minimal example with steps on how to reproduce so one can verify your change works.

Make sure you've run and fixed any issues with these commands:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace --features=extra to check that all the tests pass

@kubouch
Copy link
Contributor

kubouch commented Sep 3, 2022

Thanks! I purposefully kept it in for the next release for compatibility reasons. We can land it after the next release.

@kubouch
Copy link
Contributor

kubouch commented Sep 3, 2022

I'm not sure about the ignored tests. Could you try converting them to use the export-env and overlay use like the other tests? I'd like to see what errors they bring.

@kubouch
Copy link
Contributor

kubouch commented Sep 3, 2022

Oh, and the whole "Cleanup" part from the hackmd I'm planning to do after the release so people have time to transition and fix their scripts until the next-next release.

@WindSoilder
Copy link
Collaborator Author

WindSoilder commented Sep 3, 2022

I'm not sure about the ignored tests. Could you try converting them to use the export-env and overlay use like the other tests? I'd like to see what errors they bring.

Actually I've converted them to use export-env and overlay use, but after manually testing, it doesn't work as test expected:

hides_all_envs_within_scope

> module spam { export-env { let-env foo = "bar" } }; let-env foo = "foo"; overlay use spam; hide-env foo; $env.foo
foo

It doesn't failed with did you mean message

use_env_import_after_hide

> module spam { export-env {let-env foo = "foo" } }; overlay use spam; hide-env foo; overlay use spam; $env.foo
Error: nu::shell::name_not_found (link)

  × Name not found
   ╭─[entry #1:1:1]
 1 │ module spam { export-env {let-env foo = "foo" } }; overlay use spam; hide-env foo; overlay use spam; $env.foo
   ·                                                                                                           ─┬─
   ·                                                                                                            ╰── did you mean 'HOME'?
   ╰────

It doesn't output foo.

remove_overlay_keep_discard_overwritten_env

❯ cat spam.nu
export def foo [] { "foo" }

export alias bar = "bar"

export-env { let-env BAZ = "baz" } }

~
❯ overlay use spam.nu; let-env BAZ = `bagr`; overlay hide --keep-custom spam; $env.BAZ
bagr

It doesn't failed with did you mean message


Oh, I forget update these ignored tests, updated it.

@fdncred fdncred marked this pull request as draft September 8, 2022 13:16
@kubouch
Copy link
Contributor

kubouch commented Sep 20, 2022

@WindSoilder If you don't mind, I'll push some changes to your PR. I'd like to check the tests and also investigate the --keep-custom flag and it's easier to for me to fix up these things myself.

@kubouch
Copy link
Contributor

kubouch commented Sep 20, 2022

I went through the tests again because the use command adopted the source-env functionality. I also finished all the environment cleanup.

The virtualenv tests are failing because hide no longer hides environment variables. We need to wait until pypa/virtualenv#2422 is merged.

@kubouch kubouch marked this pull request as ready for review September 25, 2022 16:52
@kubouch kubouch merged commit b47bd22 into nushell:main Sep 25, 2022
@kubouch kubouch changed the title Removes export env command Remove deprecated environment functionality Sep 25, 2022
@WindSoilder WindSoilder deleted the no_export_env branch September 26, 2022 02:26
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

2 participants