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

style: Reorganize config file #1789

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

style: Reorganize config file #1789

wants to merge 1 commit into from

Conversation

arcuru
Copy link
Sponsor Contributor

@arcuru arcuru commented Feb 27, 2024

This is intended to be more of an example/discussion than a PR that should be merged. This PR format just may be a bit easier to make comments on, but we can move discussion somewhere else.

The config file is getting pretty unwieldy. Ellie has been pushing towards adding some more structure to it, but I think we need to re-organize some of the existing options.

There are a lot of contributions lately that are adding new options, I think this is a good time to make sure there is a cohesive plan to the options layout.

We have several logical groups of options that can pretty cleanly be grouped together. The most important is I think the keybindings. We can move the --disable-up-arrow option into the config file, more cleanly customize the filter/search/workspace modes, and have a switch for #798 if that gets implemented.

@ellie, feel free to close if you already had your own goals here :)

Checks

  • I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
  • I have checked that there are no existing pull requests for the same thing

@tessus
Copy link
Contributor

tessus commented Feb 27, 2024

I think adding more structure is a great idea. I can speak from experience that doing it without being a breaking change can be challenging. Additionally I want to mention that allowing "old" config files to be read - for a longer period of time (e.g. several releases) - is quite the maintenance burden.

I am not saying that one should just go cold turkey and introduce a new structure without an (automated) migration path.
What I am saying is that the migration path should be part of the discussion.

Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

Generally for this!

More thoughts

1. Nest everything under a history table.

I've mentioned in another comment somewhere that I'd like to start moving history-specific implementations to a new crate (atuin-history) It would make sense for it to have its own history struct, which could go under its own table.

Bearing in mind that at present there's also alias + kv syncing, which aren't yet widely used but this would allow for future flexibility.

2. Switch to KDL

Not necessarily a prerequisite, but I've been thinking about it more lately. IMO KDL seems much nicer for config than TOML is, and is much more flexible.

Spec: https://github.com/kdl-org/kdl
Rust implementation: https://github.com/kdl-org/kdl-rs

It's already used by Zellij, if you like an example: https://zellij.dev/documentation/configuration

TOML was something I never really thought about in the beginning, but in hindsight it's not the best choice. Nesting is non-obvious (order matters, rather than indentation). I also find large TOML config files difficult to read.

Another option could be something executable. EG, see how NeoVim can be configured with Lua. I can still see benefits there, but think that our config should be very quickly/easily parsed and not require executing. We can always have plugins in the future that can manipulate config.

3. Migration

Realistically, having v2 as a replacement rather than a toggle at the top may be better.

The migration path is fairly smooth then, as we can install the v2 config with defaults automatically for all new users, and use it to override old config for existing users. It could potentially be auto-generated.

The requirement here would be that it has to live under a different path.

config.kdl could work if we change format, otherwise settings.kdl may also be nice.

## address of the sync server
# sync_address = "https://api.atuin.sh"
## Timeout (in seconds) for acquiring a local database connection (sqlite)
# local_timeout = 5
Copy link
Member

Choose a reason for hiding this comment

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

tbh I'd like to eliminate all top-level/root config items

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think that all config settings should belong to a [section]. If one is not sure, it's still possible to create a Misc or Other section. On the other hand, if it is not clear, maybe the config option is not useful in the first place (with exceptions of course).

## the accepted values are identical to those of "filter_mode"
## leave unspecified to use same mode set in "filter_mode"
# filter_mode_shell_up_key_binding = "global"
[sync]
Copy link
Member

Choose a reason for hiding this comment

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

may also want

enabled = true/false

some users seem to want confirmation that Atuin is offline.

## number of context lines to show when scrolling by pages
# scroll_context_lines = 1
# Customize any option in the ui table here
# [keybindings.ctrl_r.ui]
Copy link
Member

Choose a reason for hiding this comment

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

I like this way of describing things! Much neater.

@YummyOreo
Copy link
Contributor

YummyOreo commented Mar 4, 2024

What if we separated config for the tui (i.e vim mode) and config for atuin itself (i.e sync freq) into their own files?

@@ -108,10 +117,25 @@
## start with ^ or end with $, they'll match anywhere in CWD.
## For details on the supported regular expression syntax, see
## https://docs.rs/regex/latest/regex/#syntax
# cwd_filter = [
# cwd = [
Copy link
Contributor

Choose a reason for hiding this comment

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

cmd and cwd look very similar. Also, one doesn't have to write these more than once, thus I believe that the following are more descriptive (for non-experts):

command or commands
and
directory or directories

@atuin-bot
Copy link

This pull request has been mentioned on Atuin Community. There might be relevant details there:

https://forum.atuin.sh/t/settings-changes/308/1

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

5 participants