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

Override config path #141

Closed
wants to merge 3 commits into from
Closed

Conversation

niklasmohrin
Copy link
Collaborator

Closes #107

This is a first sketch that satisfies the requested behavior.
However, I would like to move the logic into get_config_path, so that
all the other config file related functions use the flag as well.

I (for now) decided, that the program should exit, if the given path is not a
file, instead of falling back to a default configuration.

Furthermore, I think there is some work around the naming of all the flags.
I think, that the most appropriate name for this new flag would be
--config-path, which is already taken though. In my opinion, there should
be something like --show-config-path for that, but this renaming (if desired)
can also wait until there are other (or more significant) breaking changes.

Let me know what you think!

@niklasmohrin
Copy link
Collaborator Author

If the logic around the flag moves into get_config_path, I could also write some tests around that.

@dbrgn
Copy link
Owner

dbrgn commented Jan 29, 2021

Yeah, --config-path is unfortunate. I'd actually prefer to replace that with a --show-paths that works kind of like this:

$ tldr --show-paths
Config path:      /home/danilo/.config/tealdeer/config.toml
Cache dir:        /home/danilo/.cache/tealdeer/
Custom pages dir: /home/danilo/.local/share/tealdeer/pages/

(I'm not yet 100% sure if we should include the cache dir, or the pages dir instead. Maybe the latter would be better?)

What do you think?

If we have that, we could probably re-purpose --config-path to point to the desired config instead. I don't think it's that much of a breaking change, since people will probably not really use that flag in day-to-day usage. It's mostly a shortcut for getting started.

(Docs would need to be updated as well.)

@niklasmohrin
Copy link
Collaborator Author

Yeah, --show-paths sounds right. I think the cache directory might as well be included, seems like a perfect place to report it to the user to me

@dbrgn
Copy link
Owner

dbrgn commented Jan 30, 2021

By the way, why not just --config <path> for this PR?

@niklasmohrin
Copy link
Collaborator Author

I guess that would work too and be easier to type 😄

@dbrgn
Copy link
Owner

dbrgn commented Jan 30, 2021

See #162! If you agree with that, the config directory should be returned together with PageSource::ConfigVar if it was overridden in the config.

@niklasmohrin niklasmohrin marked this pull request as draft February 6, 2021 17:30
@niklasmohrin
Copy link
Collaborator Author

@dbrgn I started fresh with implementing this again and I was wondering what variable the flag should target here. In the old implementation (which I left in as a commit for reference) I had it so that if the given path was a directory, the config directory would be overridden and otherwise it would be just the config file location. Right now, I only implemented file overrides for the exact location, because I find the old logic a bit unintuitive by now 😄

If I had to decide which of the two I would want a flag for, I would choose the directory, because then the config path would be relative to that too and future tealdeer files would not have to get their own override flag. However, this can already be done with the environment variable and would not allow to name the tealdeer config file anything other than config.toml (which I believe is the main reason this flag is needed, right?).

Maybe this should really be a new environment variable TEALDEER_CONFIG_FILENAME or so. This would allow the same things as the flag, but it would be more consistent with what's already possible (and allow for choosing from different files in the same directory). Finally, off the top of my head, I don't know of any situation where you can spawn a process with a flag but not with a env var.

What do you think is best to do here?

@dbrgn dbrgn mentioned this pull request Apr 7, 2021
@dbrgn
Copy link
Owner

dbrgn commented Apr 14, 2021

I had it so that if the given path was a directory, the config directory would be overridden and otherwise it would be just the config file location.

I wouldn't implement any file-or-dir detection. Either the user should specify the file path, or the directory path. Other logic could be confusing and would make maintenance harder.

If I had to decide which of the two I would want a flag for, I would choose the directory, because then the config path would be relative to that too and future tealdeer files would not have to get their own override flag. However, this can already be done with the environment variable and would not allow to name the tealdeer config file anything other than config.toml (which I believe is the main reason this flag is needed, right?).

Yep, I agree. I would rather specify the config file. The reasoning why the main config file should be "special" over other potential config files is that everything else can be configured from within the config file, but not the config file path itself. I think that env vars like TEALDEER_CACHE_DIR should have been part of the config file.

With a file path (vs a dir path) we could also in theory implement support for reading the config from stdin, or from any other file-like object.

My suggestion: A parameter --config <config-file-path> that overrides the config file path. If the file does not exist, return an error (do not fall back to the default path).

@dbrgn dbrgn added this to the v1.5.0 milestone Apr 14, 2021
@dbrgn dbrgn added the waiting-for-author The PR needs an update before it can be considered for merging. label Sep 7, 2021
@dbrgn dbrgn modified the milestones: v1.5.0, v1.6.0 Dec 4, 2021
@niklasmohrin
Copy link
Collaborator Author

(this is currently waiting until #212 is done to make a design choice here)

@niklasmohrin
Copy link
Collaborator Author

This PR is wildly out of date, a fresh start is probably easier at this point

@niklasmohrin niklasmohrin removed this from the v1.7.0 milestone Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-author The PR needs an update before it can be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Override config file
2 participants