Include custom pages directory in --show-paths command #184
Conversation
src/main.rs
Outdated
| .as_ref() | ||
| .to_str() | ||
| .map(ToString::to_string) | ||
| .unwrap_or_else(|| String::from("[Invalid]")); |
There was a problem hiding this comment.
Although I think I may have proposed this some time ago, you should be able to just use a &str here. I don't think the unwrap is proper here, if this variable would really always be Some, the argument shouldn't be an option. Some analysis is needed here
I am a bit confused by this function again (also outside of your diff). It feels a bit like a lot of similar pieces are coming together here, but all the information is gathered in different ways. I am not up to speed on how config / argument / env var merging is handled in tealdeer, but I feel like this should really be a method on some big instance of Configuration (like a singleton, but not global?) or similar that knows all the final settings. This has nothing to do with your diff though, I'm just spiraling 😆
There was a problem hiding this comment.
It was because I had to use get_app_root in the Default for RawDirectoriesConfig and I had I discussion with @dbrgn about whether or not the RawDirectoriesConfig::default() should panic if we unwrapped get_app_root.
I'll try to find that discussion
Original Question: #142 (comment)
Discussion about it: #142 (comment)
There was a problem hiding this comment.
It appears that I had made the decision to use option, I think I'm going to go back and change it now that I realize how unintuitive this is.
There was a problem hiding this comment.
Well, yes. But the show_paths function requires the option to be Some, so it could just take the value. The caller would then have the unwrap or even better, an expect. Am I missing something?
| @@ -151,15 +151,18 @@ impl Default for RawUpdatesConfig { | |||
| #[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] | |||
| struct RawDirectoriesConfig { | |||
| #[serde(default)] | |||
| pub custom_pages_dir: Option<PathBuf>, | |||
| pub custom_pages_dir: PathBuf, | |||
There was a problem hiding this comment.
I am not really sure if my understanding is correct here, but I thought that all the Raw*Configs are just "dumb" data containers, that are "materialized" into meaningful values (with smart defaults and all) when converting to their *Config counterparts. Then again, this happened here before 🤷♂️
@dbrgn why exactly do we have this distinction again?
There was a problem hiding this comment.
Yeah I'm not sure either tbh. Kinda reminds me of the DTO pattern, since RawConfig is the one that's serializable and Config is not.
There was a problem hiding this comment.
If PathBuf is not an Option, then deserializing a TOML config file without a custom_pages_dir specified will fail.
(Actually, in this case it will use the default value of PathBuf due to the annotation, but I'm not sure what that is, and I don't think it's meaningful.)
@niklasmohrin is right, Raw*Config is purely here for deserializing the config files into Rust-y values, and then the non-raw config objects contain validated and potentially more structured values.
There was a problem hiding this comment.
@dmaahs2017 can you revert the removal of the options? otherwise we cannot differentiate between someone not setting the path in the config at all (which is valid of course) and someone setting an empty path (which is invalid).
With your change, if I'm not mistaken, the default value will be used if the custom pages dir is not set by the user (which is an empty path). This empty path will be joined with a filename. This means that a user that does not set the custom pages dir, but has a tldr.tar file in the current directory, will be served this local file instead of the actual tar page.
There was a problem hiding this comment.
Not much I can say about content, just some small oddities I found. I still find it weird that the configuration comes in so many different ways, but that's not your fault. Don't really know what to think about the config stuff until @dbrgn explains my above question and we decide how to handle this. To be frank, the config seems more convoluted to me than I imagine it has to be, but we'll have to see. That said, this is nothing that this PR has to care about really, we'll just have to wait and see 🤷♂️
- inline find patch to be at the same abstraction layer
|
👍 Looks good, just don't know about #184 (comment) |
|
@niklasmohrin fixed the merge conflicts. Could we try to push this in sooner than later? 😄 I don't think there is anything else required here. Unless we want to refactor the Config Structs somehow, in which case I'd be happy to do it in a separate PR just so we can get this functionality in. Personal Update: Been a bit busy with work and college so I haven't been quite as active as I'd like |
dbrgn
added
the
waiting-for-author
|
With the goal of getting a new release out soon: Replaced by #236 🙂 |
Adds custom pages to output of
--show-paths. See issue: #182If [directories.custom_page_dir] is not set then it will show the default location as defined by the
DirectoriesConfigRawDirectoriesConfigstructs