Fix issues in the example configs by PerchunPak · Pull Request #14601 · nushell/nushell

PerchunPak · 2024-12-16T18:39:16Z

For some reason, it had multiple syntax errors and other issues, like undefined options. Would be great to add a test for sourcing all example configs, but I don't know rust

See also #14249 (comment)

CC @NotTheDr01ds

PerchunPak · 2024-12-16T18:43:05Z

crates/nu-utils/src/default_files/sample_config.nu

 #     - 'd': dimmed

-# foreground, background, and cursor colors are not handled by Nushell, but can be used by 
+# foreground, background, and cursor colors are not handled by Nushell, but can be used by


I am unsure what to do here, example configuration should work out of the box, or else it is really confusing to try different options

PerchunPak · 2024-12-16T18:43:48Z

crates/nu-utils/src/default_files/sample_config.nu

 $env.config.color_config.float       # Float value
-$env.config.color_config.glob        # Glob value (must be declared)
+# TODO: https://github.com/nushell/nushell/issues/14600
+#$env.config.color_config.glob        # Glob value (must be declared)


Probably other deleted options in this list are affected by the same linked issue

I think these are fine. They are simply lists of settings here. We don't need to show a "default" or example value for each one, IMHO. The code is uncommented so that it is syntax highlighted when viewing through nu-highlight. Ultimately, this is "documentation" that probably gets moved over to the Book itself, and these files go away.

In the best scenario, docs would be autogenerated, so #14600 couldn't happen

Also, this kind of approach (storing documentation as default config) is not really viable for something like nixos (where configs are managed by nix itself, you don't have a default config in your disposal)

@PerchunPak Agreed - That's why the only method for a user to access them is nu config --sample. The end-user (and package manager) never has access to them directly.

In the past (0.100 and before) doc and defaults were mixed. Starting in 0.101 with these changes, they no longer are.

So should I return all other deleted options?

Yes, any option that is just listed by itself without an assignment should be returned. Thanks!

PerchunPak · 2024-12-16T19:21:24Z

crates/nu-utils/src/default_files/sample_config.nu

 use std/util "path add"
 path add "~/.local/bin"
-path add ($env.CARGO_HOME | path join "bin")
+path add ($env.HOME | path join ".cargo/bin")


I don't have cargo installed, so I made it to not depend on cargo

Well, that would break my system ;-)

If you were using Cargo, then you'd need to use $env.CARGO_HOME to calculate it properly.

It's okay to have examples that don't run on everyone's system. The point here is a dynamically computed path based on an environment variable. If it's $env.HOME, that's no different than:

path add "~/.cargo/bin"

That's the exact same example as the one above it ;-)

crates/nu-utils/src/default_files/sample_config.nu

NotTheDr01ds

I think it would be good to have a PR that we can quickly approve with the typo fixes. We can sort the others out separately, but I'd like to make sure your typo corrections get into 0.101.

NotTheDr01ds · 2024-12-16T19:18:31Z

crates/nu-utils/src/default_files/sample_config.nu

 $env.config.plugin_gc.default
+# true to enable stopping of inactive plugins
 $env.config.plugin_gc.default.enabled
+# how long to wait after a plugin is inactive to stop it


Suggested change

# how long to wait after a plugin is inactive to stop it

# How long to wait after a plugin is inactive before stopping it

NotTheDr01ds · 2024-12-16T19:19:07Z

crates/nu-utils/src/default_files/sample_config.nu

+# Configuration for plugin garbage collection
 $env.config.plugin_gc
 $env.config.plugin_gc.default
+# true to enable stopping of inactive plugins


Suggested change

# true to enable stopping of inactive plugins

# true to enable stopping inactive plugins

I know this was the original text that I left in from the original config - Just noticing it could be improved as long as it's getting moved :-)

NotTheDr01ds · 2024-12-16T19:22:20Z

crates/nu-utils/src/default_files/sample_config.nu

+# before your config.nu loads), the path variable is a list that can
 # be appended to using, for example:
-$env.path ++= "~/.local/bin"
+$env.PATH ++ [ "~/.local/bin" ]


Suggested change

$env.PATH ++ [ "~/.local/bin" ]

$env.PATH ++= [ "~/.local/bin" ]

Right?

These worked when I wrote them. Then the ++ and ++= operators were changed ;-).

Huh, it used to raise a syntax error but no longer

NotTheDr01ds · 2024-12-16T19:23:24Z

crates/nu-utils/src/default_files/scaffold_env.nu

@@ -1,5 +1,7 @@
 # env.nu
 #
+# version = "0.100.1"


There's probably a buildscript somewhere that needs to be updated in order to do this. Otherwise it's always going to be out-of-date.

When I get back to my desk I'll try to figure this out.

But I'm not sure we should put a version in a file that simply has comments anyway :-)

NotTheDr01ds · 2024-12-16T19:27:06Z

crates/nu-utils/src/default_files/sample_config.nu

 $env.config.color_config.float       # Float value
-$env.config.color_config.glob        # Glob value (must be declared)
+# TODO: https://github.com/nushell/nushell/issues/14600
+#$env.config.color_config.glob        # Glob value (must be declared)


@PerchunPak Agreed - That's why the only method for a user to access them is nu config --sample. The end-user (and package manager) never has access to them directly.

In the past (0.100 and before) doc and defaults were mixed. Starting in 0.101 with these changes, they no longer are.

NotTheDr01ds · 2024-12-16T19:33:04Z

crates/nu-utils/src/default_files/sample_config.nu

-$env.config.hooks.display_output = ""      # Before Nushell output is displayed in the terminal
-$env.config.hooks.command_not_found = []   # When a command is not found
+# Before each prompt is displayed
+$env.config.hooks.pre_prompt = [{ null }]


I can understand moving the comments for consistency, but why the null additions?

Those were in 0.100.0 example config, and because $env.config.hooks.env_change had wrong type, I decided to return default value as an example. I can remove all nulls if you prefer so

NotTheDr01ds · 2024-12-16T21:06:18Z

I think we should add something to the top along the lines of:

# This file is not intended to be run or sourced in Nushell. It is for documentation purposes only. Running this file *will* generate errors.

PerchunPak · 2024-12-16T21:26:34Z

Shouldn't NU_LIB_DIRS and NU_PLUGIN_DIRS be in env.nu?

NotTheDr01ds · 2024-12-16T22:51:44Z

Shouldn't NU_LIB_DIRS and NU_PLUGIN_DIRS be in env.nu?

Not any longer. As primarily constants now, they can be set and used in the same file. There's really nothing that we have to set in env.nu any longer, so we just moved the entire "sample" over to sample_config.nu.

PerchunPak · 2024-12-17T11:42:25Z

Not any longer. As primarily constants now, they can be set and used in the same file. There's really nothing that we have to set in env.nu any longer, so we just moved the entire "sample" over to sample_config.nu.

For me, they didn't apply inside config.nu until I moved them to env.nu (source nu_scripts/... didn't work inside config.nu but did work in the interactive shell)

fdncred · 2024-12-17T12:52:24Z

For me, they didn't apply inside config.nu until I moved them to env.nu (source nu_scripts/... didn't work inside config.nu but did work in the interactive shell)

They work for me on the latest main. I just tested again.

fdncred · 2024-12-17T13:00:05Z

I also think all config files should have a version number to show which version of nushell they're compatible with.

fdncred

If this is a "sample" people will put it in place as the config and try it out. This file should work. I've added a few comments where it doesn't work but there are more than I listed.

Release is quickly approaching and we need to get this resolved.

fdncred · 2024-12-17T13:01:58Z

crates/nu-utils/src/default_files/default_config.nu

@@ -1,6 +1,4 @@
 # Nushell Config File
-#
-# version = "0.100.1"


add versions back and add them to all files please.

fdncred · 2024-12-17T13:05:10Z

crates/nu-utils/src/default_files/sample_config.nu

 # ---------------
 # Per-plugin configuration. See https://www.nushell.sh/contributor-book/plugins.html#configuration.
-plugins: {}
 $env.config.plugins


all keys should have values - also on $env.config.table.abbreviated_row_count above, as well as rows below.

NotTheDr01ds · 2024-12-17T13:20:13Z

If this is a "sample" people will put it in place as the config and try it out. This file should work. I've added a few comments where it doesn't work but there are more than I listed.

Release is quickly approaching and we need to get this resolved.

Ack - No. This is documentation. This was never designed to be run. I'll change the name of the file and command to be more clear on that.

fdncred · 2024-12-17T14:38:20Z

If this is a "sample" people will put it in place as the config and try it out. This file should work. I've added a few comments where it doesn't work but there are more than I listed.
Release is quickly approaching and we need to get this resolved.

Ack - No. This is documentation. This was never designed to be run. I'll change the name of the file and command to be more clear on that.

If we don't want people trying to use these files, which seems like they intuitively would, given the file names, then the files should have documentation (or something other than sample/example) in the name and the first lines of the file should state that it's documentation and not intended to be used as a configuration file. If they're going to be documentation, hopefully they'll be complete, otherwise I'm not sure what purpose they serve. e.g. I'm not sure how helpful it is to say "here's some of the settings you can use" vs "this is a list of all settings and what they mean".

NotTheDr01ds · 2024-12-17T15:09:51Z

@PerchunPak The confusion on this seems to likely be due to my poor choice of naming :-/

I definitely never intended these to be used as actual configuration files - They are simply doc. I've just submitted #14608 to rename these from sample_config.nu to doc_config.nu (etc.) along with updates to config nu --doc to match.

While doing this, I went ahead and pulled in most of your changes so that we can get this nailed down before the feature freeze in two days. Thank you, Thank you, Thank you! for finding these.

About the only change I didn't make was updating indentation to 4 spaces consistently. We haven't settled on that yet, and are leaning towards 2 spaces for Nushell code-style. I know there are inconsistencies right now, but I'll update them when we land on a decision there.

PerchunPak · 2024-12-17T16:09:59Z

For me, they didn't apply inside config.nu until I moved them to env.nu (source nu_scripts/... didn't work inside config.nu but did work in the interactive shell)

They work for me on the latest main. I just tested again.

const NU_LIB_DIRS must come before source... But in the sample file they are at the bottom, should I move it?

For some reason, sample config had multiple syntax errors and other issues, like undefined options. Other configs had minor issues like spaces at the end of line.

PerchunPak · 2024-12-17T16:15:00Z

@NotTheDr01ds should be done, except for #14601 (comment) (but I feel that #14608 is more suitable for this change)

NotTheDr01ds · 2024-12-17T16:29:16Z

@fdncred I think the last commit gets the actual errors that were in the documentation resolved.

If you are ready to merge it, I'll rebase #14608 on it (hopefully this afternoon) so that we reduce (hopefully eliminate) any confusion on how these files are to be used.

fdncred · 2024-12-17T16:43:31Z

ok, thanks.

@fdncred

# Description With great thanks to @fdncred and especially @PerchunPak (see #14601) for finding and fixing a number of issues that I pulled in here due to the filename changes and upcoming freeze. This PR primarily fixes a poor wording choice in the new filenames and `config` command options. The fact that these were called `sample_config.nu` (etc.) and accessed via `config --sample` created a great deal of confusion. These were never intended to be used as-is as config files, but rather as in-shell documentation. As such, I've renamed them: * `sample_config.nu` becomes `doc_config.nu` * `sample_env.nu` becomes `doc_env.nu` * `config nu --sample` becomes `config nu --doc` * `config env --sample` because `config env --doc` Also the following: * Updates `doc_config.nu` with a few additional comment-fixes on top of @PerchunPak's changes. * Adds version numbers to all files - Will need to update the version script to add some files after this PR. * Additional doc on plugin and plugin_gc configuration which I had failed to previously completely update from the older wording * Updated the comments in the `scaffold_*.nu` files to point people to `help config`/`help nu` so that, if things change in the future, it will become more difficult for the comments to be outdated. * # User-Facing Changes Mostly doc. `config nu` and `config env` changes update new behavior previously added in 0.100.1 # Tests + Formatting - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib` # After Submitting * Update configuration chapter of doc * Update the blog entry on migrating config * Update `bump-version.nu`

This file is not made accessible to the user through any of our `config` commands. Thus I discussed with Douglas to delete it, to ensure it doesn't go out of date (the version added with nushell#14601 was not yet part of the bumping script) All the necessary information on how to setup a `login.nu` file is provided in the website documentation

This file is not made accessible to the user through any of our `config` commands. Thus I discussed with Douglas to delete it, to ensure it doesn't go out of date (the version added with #14601 was not yet part of the bumping script) All the necessary information on how to setup a `login.nu` file is provided in the website documentation

PerchunPak force-pushed the fix-issues-in-sample-config branch from 6a773bd to 4d2e16e Compare December 16, 2024 18:40

PerchunPak changed the title ~~Fix issues in the sample config~~ Fix issues in the example configs Dec 16, 2024

PerchunPak force-pushed the fix-issues-in-sample-config branch from 4d2e16e to 2b4cd1c Compare December 16, 2024 18:45

PerchunPak commented Dec 16, 2024

View reviewed changes

PerchunPak force-pushed the fix-issues-in-sample-config branch from 2b4cd1c to ad4b24b Compare December 16, 2024 19:01

PerchunPak commented Dec 16, 2024

View reviewed changes

crates/nu-utils/src/default_files/sample_config.nu Outdated Show resolved Hide resolved

PerchunPak force-pushed the fix-issues-in-sample-config branch 2 times, most recently from d021982 to 4a20783 Compare December 16, 2024 19:31

NotTheDr01ds reviewed Dec 16, 2024

View reviewed changes

PerchunPak force-pushed the fix-issues-in-sample-config branch 2 times, most recently from cbaa2ec to 2747ec2 Compare December 16, 2024 20:00

PerchunPak force-pushed the fix-issues-in-sample-config branch 3 times, most recently from 160945e to eb54aaf Compare December 16, 2024 21:19

fdncred requested changes Dec 17, 2024

View reviewed changes

NotTheDr01ds mentioned this pull request Dec 17, 2024

Doc file fixes #14608

Merged

PerchunPak force-pushed the fix-issues-in-sample-config branch from eb54aaf to 604812f Compare December 17, 2024 16:04

Fix issues in the example configs

8d555fd

For some reason, sample config had multiple syntax errors and other issues, like undefined options. Other configs had minor issues like spaces at the end of line.

PerchunPak force-pushed the fix-issues-in-sample-config branch from 604812f to 8d555fd Compare December 17, 2024 16:13

fdncred merged commit cc4da10 into nushell:main Dec 17, 2024

github-actions bot added this to the v0.101.0 milestone Dec 17, 2024

PerchunPak deleted the fix-issues-in-sample-config branch December 17, 2024 16:44

fdncred added the A:configuration Issue related to nu's configuration label Dec 18, 2024

sholderbach mentioned this pull request Dec 19, 2024

Remove unused sample_login.nu file #14632

Merged

	# how long to wait after a plugin is inactive to stop it
	# How long to wait after a plugin is inactive before stopping it

	# true to enable stopping of inactive plugins
	# true to enable stopping inactive plugins

	$env.PATH ++ [ "~/.local/bin" ]
	$env.PATH ++= [ "~/.local/bin" ]

Conversation

PerchunPak commented Dec 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NotTheDr01ds Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NotTheDr01ds Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NotTheDr01ds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NotTheDr01ds commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PerchunPak commented Dec 16, 2024

Uh oh!

NotTheDr01ds commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PerchunPak commented Dec 17, 2024

Uh oh!

fdncred commented Dec 17, 2024

Uh oh!

fdncred commented Dec 17, 2024

Uh oh!

fdncred left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NotTheDr01ds commented Dec 17, 2024

Uh oh!

fdncred commented Dec 17, 2024

Uh oh!

NotTheDr01ds commented Dec 17, 2024

Uh oh!

NotTheDr01ds Dec 16, 2024 •

edited

Loading

NotTheDr01ds Dec 16, 2024 •

edited

Loading

NotTheDr01ds commented Dec 16, 2024 •

edited

Loading

NotTheDr01ds commented Dec 16, 2024 •

edited

Loading

NotTheDr01ds commented Dec 17, 2024 •

edited

Loading