More tables would be sensible, and new features should be properly fit into tables (as all so far have been)
I think there’s a midground we can achieve more easily/quickly where we just shuffle the existing settings around a bit, and leave the existing options where they are. Just remove them from the default config, and mark them as deprecated in the docs.
It may also be nice to mirror the crate structure with settings, as this is already grouped in a way that makes more sense. It would also reduce the dependencies a bit.
I think that makes sense. Maybe a mapping document (markdown table) would be a good start. After having it confirmed, the code changes should be easier to manage. What do you think? I can give the mapping table a try.
I have never seen or used an application that uses KDL, so I have to look into it. What I can see from the docs, it’s not as legible as TOML or YAML.
This is a preliminary table and is supposed to be the starting point.
I have 3 questions/suggestions (already discussed)
it could make sense to use another table for history related settings (I currently put those under client)
sync_address and sync_frequency: I’m wondering whether they should be under [sync] as server and frequency and then be used by either the daemon or the client (depending on what is used). afaik they are never used at the same time anyway.
update_check: no clue (is this also used when running the server/daemon or only when the client is used?)
Agreed, that aligns with dotfiles/etc. I’ll update the table with this
It’s tricky, as there are two. The difficulty is that they have slightly different meaning. A sync_freq of 0 outside the daemon means “sync immediately after the user runs a command”, but in the daemon acts as a timer. So if you set it to 0 there, you’re pretty much just constantly hitting the server.
It might make sense to name them differently. daemon.sync_interval, and sync.frequency, perhaps. The latter is ignored when the daemon is enabled.
It’s actually used by the TUI, but I think makes sense as a more general setting. Perhaps under client
I did not have this specific setting in mind. IMO, db_path is a client setting. Even though it is also used by the daemon, it is primarily a client setting, because it is used on the client. (e.g. if you run a server, this db is never used.)
Those are the settings I thought might be better under a table history:
Old Table
Old Setting
New Table
New Setting
history_format
history
format
history_filter
history
command_filter
cwd_filter
history
directory_filter
secrets_filter
history
secrets_filter
As a fellow architect/designer/dev I do understand this distinction very well. However, I do not think this distinction should be used in the naming of the settings. It might only confuse users. The specifics can be explained in the documentation. But it all depends whether you move the address and the frequency/interval to the [sync] table.
If you used the address under [sync], and used sync_frequency in [client] and sync_interval in [daemon], it could work. Personally I’d rather move those 2, or better yet only use one (either frequency or interval), under [sync].
Then you can also get rid of the sync_ prefix in the setting name(s).
Currently there is only one setting under [sync]. For me it looks a bit weird to have also sync_xxx settings, even though there’s a sync table.
This is probably very subjective, so never mind in case this sounds stupid to you.
db_path refers to the shell history database. There are actually other sqlite files, such as the one used for records. Without putting it in the history table, it’s pretty ambiguous
Putting record_store_path under client does make sense
I don’t think this logic makes sense. We already have a separate config file for server settings. A history table makes more sense than a client one, really - almost everything I can see under client would work better elsewhere
network_timeout is more of a sync setting, as is session_path
secrets_filter makes sense under history, as do history_format, history_filter, cwd_filter - sounds like we agree there
local_timeout is pretty ambiguous and would make sense under client
Beyond just a settings change, but I do agree with you that having only one would be nice.
Let’s just go with sync.frequency, and set some sort of minimum in the daemon. We could someday add sync.real_time to enable “sync right after a command” in the daemon too, that people try and achieve with sync_frequency=0
The history format is comprised of several columns (variables), command being one of them. So I am not sure why the history_format should be changed to command_format. This would be the same as naming it duration_format.
Edit: Supported variables in history format:
{command}, {directory}, {duration}, {user}, {host} and {time}
