Skip to content

Error out when config.nu has no editor configured#8282

Merged
fdncred merged 1 commit intonushell:mainfrom
dmatos2012:error-if-nano-not-exist
Mar 9, 2023
Merged

Error out when config.nu has no editor configured#8282
fdncred merged 1 commit intonushell:mainfrom
dmatos2012:error-if-nano-not-exist

Conversation

@dmatos2012
Copy link
Copy Markdown
Contributor

@dmatos2012 dmatos2012 commented Mar 1, 2023

Description

Fixes #8245. Instead of trying to use nano or notepad as defaults, it errors out if finds that buffer_editor , $EDITOR, $VISUAL do not exist.

If the PR is landed, Ill update the website as it means what its in there is no longer correct.

❯ config nu
Error: 
  × No editor configured
   ╭─[entry #3:1:1]
 1 │ config nu
   · ────┬────
   ·     ╰── Please specify one via environment variables $EDITOR or $VISUAL
   ╰────
  help: Nushell's config file can be found with the command: $nu.config-path. For more help: (https://nushell.sh/book/configuration.html#configurations-with-built-in-commands)

User-Facing Changes

Tests + Formatting

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 -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 2, 2023

Codecov Report

Merging #8282 (71a83f2) into main (438062d) will decrease coverage by 0.39%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8282      +/-   ##
==========================================
- Coverage   68.11%   67.72%   -0.39%     
==========================================
  Files         620      620              
  Lines       99288    99296       +8     
==========================================
- Hits        67633    67252     -381     
- Misses      31655    32044     +389
Impacted Files Coverage Δ
crates/nu-command/src/env/config/config_env.rs 41.66% <0.00%> (ø)
crates/nu-command/src/env/config/config_nu.rs 41.66% <0.00%> (ø)
crates/nu-command/src/env/config/utils.rs 0.00% <0.00%> (ø)
crates/nu-color-config/src/nu_style.rs 9.31% <0.00%> (-70.92%) ⬇️
crates/nu-parser/src/parser.rs 82.47% <0.00%> (-0.16%) ⬇️

@rgwood
Copy link
Copy Markdown
Contributor

rgwood commented Mar 2, 2023 •
edited
Loading

Thanks for the PR! I think it's good to return an error here.

I would vote to keep Notepad as a fallback for Windows, but I don't feel too strongly about that.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Mar 9, 2023

Let's go ahead with this. With the latest versions of Windows, notepad is also distributed on the Microsoft Store. Conceivably, they could stop distributing it with the OS at some point. However, I'm not sure what Microsoft's plans are around this. Either way, having this perform consistently on all platforms seems ok.

@fdncred fdncred merged commit ccd72fa into nushell:main Mar 9, 2023
@fdncred fdncred mentioned this pull request May 13, 2023
Closed
@dmatos2012 dmatos2012 deleted the error-if-nano-not-exist branch October 24, 2023 20:15
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.

config nu: use vi or vim instead if nano does not exist

3 participants

@dmatos2012 @rgwood @fdncred