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

Include custom pages directory in --show-paths command #184

Closed
21 changes: 7 additions & 14 deletions src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,20 +178,14 @@ impl Cache {
.find(|path| path.exists() && path.is_file())
}

/// Look up custom patch (<name>.patch). If it exists, store it in a variable.
fn find_patch(patch_name: &str, custom_pages_dir: Option<&Path>) -> Option<PathBuf> {
custom_pages_dir
.map(|custom_dir| custom_dir.join(patch_name))
.filter(|path| path.exists() && path.is_file())
}

/// Search for a page and return the path to it.
pub fn find_page(
&self,
name: &str,
languages: &[String],
custom_pages_dir: Option<&Path>,
custom_pages_dir: impl AsRef<Path>,
) -> Option<PageLookupResult> {
let custom_pages_dir = custom_pages_dir.as_ref();
let page_filename = format!("{}.md", name);
let patch_filename = format!("{}.patch", name);
let custom_filename = format!("{}.page", name);
Expand All @@ -217,14 +211,13 @@ impl Cache {
.collect();

// Look up custom page (<name>.page). If it exists, return it directly
if let Some(config_dir) = custom_pages_dir {
let custom_page = config_dir.join(custom_filename);
if custom_page.exists() && custom_page.is_file() {
return Some(PageLookupResult::with_page(custom_page));
}
let custom_page = custom_pages_dir.join(custom_filename);
if custom_page.is_file() {
return Some(PageLookupResult::with_page(custom_page));
}

let patch_path = Self::find_patch(&patch_filename, custom_pages_dir);
// Look up custom patch (<name>.patch). If it exists, store it in a variable.
let patch_path = Some(custom_pages_dir.join(&patch_filename)).filter(|p| p.is_file());

// Try to find a platform specific path next, append custom patch to it.
if let Some(pf) = self.get_platform_dir() {
Expand Down
14 changes: 9 additions & 5 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,15 +151,19 @@ 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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

}

impl Default for RawDirectoriesConfig {
fn default() -> Self {
Self {
custom_pages_dir: get_app_root(AppDataType::UserData, &crate::APP_INFO)
.map(|path| path.join("pages"))
.ok(),
custom_pages_dir: get_app_root(AppDataType::UserData, &crate::APP_INFO).map_or_else(
|_| {
eprintln!("Could not locate the App Root");
std::process::exit(1);
},
|p| p.join("pages"),
),
}
}
}
Expand Down Expand Up @@ -214,7 +218,7 @@ pub struct UpdatesConfig {

#[derive(Clone, Debug, PartialEq)]
pub struct DirectoriesConfig {
pub custom_pages_dir: Option<PathBuf>,
pub custom_pages_dir: PathBuf,
}

#[derive(Clone, Debug, PartialEq)]
Expand Down
41 changes: 21 additions & 20 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
use std::fs::File;
use std::io::BufRead;
use std::io::BufReader;
use std::path::PathBuf;
use std::path::{Path, PathBuf};
use std::process;
use std::{env, io::Write};

Expand Down Expand Up @@ -200,7 +200,7 @@ fn show_config_path() {
}

/// Show file paths
fn show_paths() {
fn show_paths(custom_pages_dir: impl AsRef<Path>) {
let config_dir = get_config_dir().map_or_else(
|e| format!("[Error: {}]", e),
|(mut path, source)| {
Expand Down Expand Up @@ -235,10 +235,13 @@ fn show_paths() {
.unwrap_or_else(|_| String::from("[Invalid]"))
},
);
println!("Config dir: {}", config_dir);
println!("Config path: {}", config_path);
println!("Cache dir: {}", cache_dir);
println!("Pages dir: {}", pages_dir);
let custom_pages_dir = custom_pages_dir.as_ref().to_str().unwrap_or("[Invalid]");

println!("Config dir: {}", config_dir);
println!("Config path: {}", config_path);
println!("Cache dir: {}", cache_dir);
println!("Pages dir: {}", pages_dir);
println!("Custom pages dir: {}", custom_pages_dir);
}

/// Create seed config file and exit
Expand Down Expand Up @@ -356,15 +359,6 @@ fn main() {
process::exit(0);
}

// Show config file and path, pass through
if args.flag_config_path {
eprintln!("Warning: The --config-path flag is deprecated, use --show-paths instead");
show_config_path();
}
if args.flag_show_paths {
show_paths();
}

// Create a basic config and exit
if args.flag_seed_config {
create_config_and_exit();
Expand Down Expand Up @@ -403,6 +397,15 @@ fn main() {
}
};

// Show config file and path, pass through
if args.flag_config_path {
eprintln!("Warning: The --config-path flag is deprecated, use --show-paths instead");
show_config_path();
}
if args.flag_show_paths {
show_paths(&config.directories.custom_pages_dir);
}

if args.flag_pager || config.display.use_pager {
configure_pager();
}
Expand Down Expand Up @@ -472,11 +475,9 @@ fn main() {
.map_or_else(get_languages_from_env, |flag_lang| vec![flag_lang]);

// Search for command in cache
if let Some(page) = cache.find_page(
&command,
&languages,
config.directories.custom_pages_dir.as_deref(),
) {
if let Some(page) =
cache.find_page(&command, &languages, &config.directories.custom_pages_dir)
{
if let Err(msg) = print_page(&page, args.flag_markdown, &config) {
eprintln!("{}", msg);
process::exit(1);
Expand Down
48 changes: 44 additions & 4 deletions tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,21 +230,57 @@ fn test_setup_seed_config() {
.stderr(contains("Successfully created seed config file here"));
}

/// This test is to show that there is a default path for custom_pages_dir if it is not defined in
/// the config.toml
#[test]
fn test_show_paths_custom_pages_not_in_config() {
use app_dirs::{get_app_root, AppDataType, AppInfo};

let testenv = TestEnv::new();
testenv
.command()
.args(&["--show-paths"])
.assert()
.success()
.stdout(contains(format!(
"Custom pages dir: {}",
get_app_root(
AppDataType::UserData,
&AppInfo {
name: "tealdeer",
author: "tealdeer"
}
)
.expect("get_app_root failed, this should never happen...")
.join("pages")
.to_str()
.expect(
"path returned from get_app_root was not valid UTF-8, this should never happen..."
)
)));
}

#[test]
fn test_show_paths() {
let testenv = TestEnv::new();

// Set custom pages directory
testenv.write_config(format!(
"[directories]\ncustom_pages_dir = '{}'",
testenv.custom_pages_dir.path().to_str().unwrap()
));

testenv
.command()
.args(["--show-paths"])
.assert()
.success()
.stdout(contains(format!(
"Config dir: {}",
"Config dir: {}",
testenv.config_dir.path().to_str().unwrap(),
)))
.stdout(contains(format!(
"Config path: {}",
"Config path: {}",
testenv
.config_dir
.path()
Expand All @@ -253,17 +289,21 @@ fn test_show_paths() {
.unwrap(),
)))
.stdout(contains(format!(
"Cache dir: {}",
"Cache dir: {}",
testenv.cache_dir.path().to_str().unwrap(),
)))
.stdout(contains(format!(
"Pages dir: {}",
"Pages dir: {}",
testenv
.cache_dir
.path()
.join("tldr-master")
.to_str()
.unwrap(),
)))
.stdout(contains(format!(
"Custom pages dir: {}",
testenv.custom_pages_dir.path().to_str().unwrap(),
)));
}

Expand Down