tealdeer-rs / tealdeer

A very fast implementation of tldr in Rust.
https://tealdeer-rs.github.io/tealdeer/
Apache License 2.0
4.17k stars 123 forks source link

Allow overriding via env vars custom pages/patches (just like config/cache) #306

Open eugenesvk opened 1 year ago

eugenesvk commented 1 year ago

Currently you can override via Environment Variables

To allow using the same config file on various machines/users, since config only accepts absolute paths while it's easier to set relative paths in a shell config

dbrgn commented 1 year ago

Hmm, when setting relative paths, what would they be relative to?

Actually the TEALDEER_CACHE_DIR is something that is a leftover from when IIRC we didn't even have a config file. Since the config now contains a directories section, that's the place where I'd like to put all directory configuration values going forward. Adding additional env variables just makes things more complex. (I'd even like to deprecate the TEALDEER_CACHE_DIR variable and recommend using the config file instead, so that TEALDEER_CONFIG_DIR is the only env variable we need to check. What do you think, @niklasmohrin? We could still keep it working for quite some time in the code, but update docs.)

If absolute paths are a problem, then maybe that's where improvements could be made. Maybe supporting ~ expansion? (Not sure how that feels on Windows though, I think on Windows we'd need to support %HOMEPATH% instead 😕)

eugenesvk commented 1 year ago

Hmm, when setting relative paths, what would they be relative to?

Relative to the global config path variable I've set up

If absolute paths are a problem, then maybe that's where improvements could be made

Sure, and the least complicated way to improve it is so support env vars, where all of the issues like ~ and %HOMEPATH are already resolved in your shell config and only require a simple $TEALDEER_CONFIG_DIR = f"{cfgH}/tldr" instead of forcing to use your resolver

niklasmohrin commented 1 year ago

That's a fair point, but like it has been already said, it's a bit difficult for us to decide where to draw the line between "worth having an env var for" and "only needed in config file"; in general, we cannot have all settings accessible only through env vars, because there are too many and we don't want to clutter user's env. But maybe we can resolve all paths through the shell for convenience - if its not too much of a performance hit

eugenesvk commented 1 year ago

cannot have all settings accessible only through env vars

That's fine because env vars don't replace any configuration files, they only allow overriding it, so if users don't want to "clutter" their envs, then they can just... not set any env vars and use the configs

Also don't understand re. the performance hit — you're just reading from an environment var, there is no path resolution?

niklasmohrin commented 1 year ago

I suppose we could think about exposing all config settings through the environment. It would be a bit tricky to find the right spot to put this logic in, but could work. The performance hit I am talking about is the reading of lots of environment variables and running all the merging logic - I don't know what to expect there though

eugenesvk commented 1 year ago

Just to clarify — I'm not asking to expose all config settings, I don't see any point in that

niklasmohrin commented 1 year ago

Well, but only having these seems arbitrary again

eugenesvk commented 1 year ago

It's not arbitrary, these env solve the issue with relative paths, what issues does having everything solve???

MatejKafka commented 4 months ago

Just wanted to chime in – having environment variables for all paths is pretty useful for portable scenarios. With env vars, I can just write a wrapper script that sets the env vars to portable paths and invokes Tealdeer, and it works correctly when I move tealdeer to another path.

When the other paths are only configurable in the config file, I must patch them whenever I move Tealdeer, which is much harder to do in a script. If the config file supported relative paths, I could set them once and then just set the single env var, so that would also work for me.

niklasmohrin commented 4 months ago

Yup agree, but as I said, I think we should come up with something unified for all options instead of adding exceptions for particular options now

MatejKafka commented 4 months ago

Thinking about it a bit more, imo the best solution would be to add support for config-relative paths in the config file, which solves the immediate issue with minimal implementation changes. Do note that the paths must be relative to the config file – adding support for ~ and other shorthands does not solve the issue of making Tealdeer portable.

In the longer run, I agree that exposing all config file options in a more scriptable way might be worthwhile, but that's a much more elaborate change. As a sidenote, adding command-line parameters seems better to me than exposing everything using environment variables, since it is easier to use interactively and should have a bit less overhead.

niklasmohrin commented 4 months ago

Sounds good to me, it also seems straightforward to do: Just Path::join the configured directory with the directory that the config file resides in. PRs (with tests) welcome