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

RFC: Redesign config structure #5

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

LhKipp
Copy link
Contributor

@LhKipp LhKipp commented Mar 18, 2021

No description provided.

@LhKipp LhKipp changed the title Add RFC v1 RFC: Redesign config structure Mar 18, 2021
text/0005-config_design.md Outdated Show resolved Hide resolved
vpn_client = "/opt/vpn_client/bin"
cargo = "/home/leo/.cargo"
```
4. Some settings are not put inside a table `[...]`. Those could at least be grouped under `[general]`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, agreed about [general]. I'm definitely for more orgainization.

- Add `package unload` => Unload a config/package
- Add `package init (path)` (Initialize package structure under path, so users don't have to look it up all the time)
- Allow passing of --local to `config rm <key>` (Remove key from last loaded local config), `config set/set_into <key>` (Set key/value into last loaded local config), `package list` (Only show local configs).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my one wish for a config command set update is that we move to toml_edit so that comments are respected and maintained. today, with our current config setting tools, comments are removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. The current config code makes a lot of assumption. It will need some substantial rework, especially after accepting this RFC.

@fdncred
Copy link

fdncred commented Mar 18, 2021

Generally speaking, i like this rfc. I think there's a lot of good points in it. i've commented on a few details. Thanks @LhKipp!!

5. With `.nu-env` and the "global" config under `$HOME/.config/nu/config.toml` being merged, we can think of `autoenv` as a local config (a config local to a directory). Therefore we could rename `.nu-env` to `.nu/config.toml`. (We may need to adapt the `autoenv` commands accordingly)
- Rename `autoenv trust` to `package trust` (see 9)
- Rename `autoenv untrust` to `package untrust` (see 9)
6. We have `startup` and `on_exit`. We could rename `startup` to `on_enter` (Better naming, better consistency (`enter`/`exit` command)).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also think about adding a on_enter.nu and on_exit.nu file right next to the config.toml. on_enter and on_exit config values don't scale past 1 line of code. Files (I think) would suit most users better.

Copy link

@fdncred fdncred Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reminds me, I've been looking at the fish shell source code recently. I noticed that most, if not all, of their built in commands have cpp code but they also all have *.fish commands too. i.e. there's cd an cd.fish. I was thinking that this could be an approach we take with nushell in our stdlib.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds cool. Is it something we need to consider when redesigning the config (and how configs should work)?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably a broader conversation about stdlib and built-in commands in general. so, we can probably drop the discussion about it since it's not specifically related to config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. But I generally agree with your opinion. It would also allow us to prototype stuff faster :).

@LhKipp
Copy link
Contributor Author

LhKipp commented Mar 20, 2021

Another thing we should consider, if we want a package system: How are default values implemented?

One approach could be: Allow a file config.defaults.<file_extension> in each package, which describes configuration default values. These values are loaded and returned by nu, if no user defined config value exists.
This approach has 2 advantages, I can see right now:

  • Users can easily and in a uniform style discover default values
  • Package authors don't have to write code like:
set write_to = $(config get where_should_my_package_write_to)
if write_to == "" {
	write_to = $HOME/default_path
} 

But instead they can write code like:

# There exists a default value for each config item, so we don't have to check
set write_to = _$(config get where_should_my_package_write_to)

@LhKipp LhKipp closed this Mar 20, 2021
@LhKipp LhKipp reopened this Mar 20, 2021
@LhKipp
Copy link
Contributor Author

LhKipp commented Mar 20, 2021

Another thing, that could be considered (but doesn't have priority imo), would be to validate configurations before loading them.
Each package could have a config.definition.<extension> file. But I ain't being sure, what the best format and what the constraints are.

@fdncred
Copy link

fdncred commented Mar 20, 2021

I'm guessing packaging is beyond a 1.0 scope but it would be good to get @jonathandturner 's and @andrasio 's take on this.

@LhKipp
Copy link
Contributor Author

LhKipp commented Mar 20, 2021

I'm guessing packaging is beyond a 1.0 scope

I don't think its that hard and I think its important. Packaging solves the problem: How can users share code / config? I think a good answer is something that user already want. Currently users can only share their scripts on Discord or by contributing to nushell/nu_scripts (Or by contributing them as commands to nushell directly)... All options are sub-optimal from my pov.

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.

2 participants