Repository: dbrgn/tealdeer (rust)

Issue:
Custom patches should not need to be formatted like a full page
Quote from PR #142 discussion:
> Another issue for a new PR: Right now the patches need to be formatted like a full page, including a heading. Otherwise, the first line is interpreted as the heading, and skipped when printing (because the page and the patch are passed to separate tokenizer instances). However, I think that too can be fixed in a separate PR, I don't want to hold up this PR too long.

Pre-requisites:
* Issue #180

Acceptance Criteria: 
* Format of .patch files should be expected to not have a heading
* Update tests to reflect this change


Pull request description:
Concatenate page and patch bytes before processing
This way a patch file does not need a well-formed header anymore.

Fixes #181 and thus the last open feature/bug issue for the 1.5.0 milestone! 🎉

Replaces #202.

Patch:
diff --git a/docs/src/SUMMARY.md b/docs/src/SUMMARY.md
index 8aaac31..8d1a634 100644
--- a/docs/src/SUMMARY.md
+++ b/docs/src/SUMMARY.md
@@ -4,7 +4,7 @@
 
 - [Installing](./installing.md)
 - [Usage](./usage.md)
-  - [Custom Pages](./usage_custom_pages.md)
+  - [Custom Pages and Patches](./usage_custom_pages.md)
 - [Configuration](./config.md)
   - [Section: \[display\]](./config_display.md)
   - [Section: \[style\]](./config_style.md)
diff --git a/docs/src/usage_custom_pages.md b/docs/src/usage_custom_pages.md
index 589479f..bdae7a9 100644
--- a/docs/src/usage_custom_pages.md
+++ b/docs/src/usage_custom_pages.md
@@ -4,7 +4,7 @@ Tealdeer allows creating new custom pages, overriding existing pages as well as
 extending existing pages.
 
 The directory, where these custom pages and patches can be placed, follows OS
-conventions. On Linux, for example, the default location is
+conventions. On Linux for instance, the default location is
 `~/.local/share/tealdeer/pages/`. To print the path used on your system, simply
 run `tldr --show-paths`.
 
@@ -33,6 +33,9 @@ want to extend it with your own examples that you frequently need. In this
 case, use a file called `<command>.patch`, it will be appended to existing
 pages.
 
+> **Note:** Because the patch file will be concatenated directly to the page
+> file, it needs to start with an empty line.
+
 Path:
 
     $CUSTOM_PAGES_DIR/<command>.patch
diff --git a/src/cache.rs b/src/cache.rs
index ac6347c..87db30c 100644
--- a/src/cache.rs
+++ b/src/cache.rs
@@ -1,9 +1,8 @@
 use std::{
     env,
     ffi::OsStr,
-    fs,
-    io::{Cursor, Read, Seek},
-    iter,
+    fs::{self, File},
+    io::{BufReader, Cursor, Read, Seek},
     path::{Path, PathBuf},
 };
 
@@ -32,8 +31,8 @@ pub struct Cache {
 
 #[derive(Debug)]
 pub struct PageLookupResult {
-    page_path: PathBuf,
-    patch_path: Option<PathBuf>,
+    pub page_path: PathBuf,
+    pub patch_path: Option<PathBuf>,
 }
 
 impl PageLookupResult {
@@ -49,8 +48,36 @@ impl PageLookupResult {
         self
     }
 
-    pub fn paths(&self) -> impl Iterator<Item = &Path> {
-        iter::once(self.page_path.as_path()).chain(self.patch_path.as_deref())
+    /// Create a buffered reader that sequentially reads from the page and the
+    /// patch, as if they were concatenated.
+    ///
+    /// This will return an error if either the page file or the patch file
+    /// cannot be opened.
+    pub fn reader(&self) -> Result<BufReader<Box<dyn Read>>, String> {
+        // Open page file
+        let page_file = File::open(&self.page_path)
+            .map_err(|msg| format!("Could not open page file at {:?}: {}", self.page_path, msg))?;
+
+        // Open patch file
+        let patch_file_opt = match &self.patch_path {
+            Some(path) => Some(
+                File::open(path)
+                    .map_err(|msg| format!("Could not open patch file at {:?}: {}", path, msg))?,
+            ),
+            None => None,
+        };
+
+        // Create chained reader from file(s)
+        //
+        // Note: It might be worthwhile to create our own struct that accepts
+        // the page and patch files and that will read them sequentially,
+        // because it avoids the boxing below. However, the performance impact
+        // would first need to be shown to be significant using a benchmark.
+        Ok(BufReader::new(if let Some(patch_file) = patch_file_opt {
+            Box::new(page_file.chain(&b"\n"[..]).chain(patch_file)) as Box<dyn Read>
+        } else {
+            Box::new(page_file) as Box<dyn Read>
+        }))
     }
 }
 
@@ -362,21 +389,53 @@ impl Cache {
 mod tests {
     use super::*;
 
+    use std::{
+        fs::File,
+        io::{Read, Write},
+    };
+
     #[test]
-    fn test_page_lookup_result_iter_with_patch() {
-        let lookup = PageLookupResult::with_page(PathBuf::from("test.page"))
-            .with_optional_patch(Some(PathBuf::from("test.patch")));
-        let mut iter = lookup.paths();
-        assert_eq!(iter.next(), Some(Path::new("test.page")));
-        assert_eq!(iter.next(), Some(Path::new("test.patch")));
-        assert_eq!(iter.next(), None);
+    fn test_reader_with_patch() {
+        // Write test files
+        let dir = tempfile::tempdir().unwrap();
+        let page_path = dir.path().join("test.page");
+        let patch_path = dir.path().join("test.patch");
+        {
+            let mut f1 = File::create(&page_path).unwrap();
+            f1.write_all(b"Hello\n").unwrap();
+            let mut f2 = File::create(&patch_path).unwrap();
+            f2.write_all(b"World").unwrap();
+        }
+
+        // Create chained reader from lookup result
+        let lr = PageLookupResult::with_page(page_path).with_optional_patch(Some(patch_path));
+        let mut reader = lr.reader().unwrap();
+
+        // Read into a Vec
+        let mut buf = Vec::new();
+        reader.read_to_end(&mut buf).unwrap();
+
+        assert_eq!(&buf, b"Hello\n\nWorld");
     }
 
     #[test]
-    fn test_page_lookup_result_iter_no_patch() {
-        let lookup = PageLookupResult::with_page(PathBuf::from("test.page"));
-        let mut iter = lookup.paths();
-        assert_eq!(iter.next(), Some(Path::new("test.page")));
-        assert_eq!(iter.next(), None);
+    fn test_reader_without_patch() {
+        // Write test file
+        let dir = tempfile::tempdir().unwrap();
+        let page_path = dir.path().join("test.page");
+        {
+            let mut f = File::create(&page_path).unwrap();
+            f.write_all(b"Hello\n").unwrap();
+        }
+
+        // Create chained reader from lookup result
+        let lr = PageLookupResult::with_page(page_path);
+        let mut reader = lr.reader().unwrap();
+
+        // Read into a Vec
+        let mut buf = Vec::new();
+        reader.read_to_end(&mut buf).unwrap();
+
+        assert_eq!(&buf, b"Hello\n");
     }
 }
diff --git a/src/main.rs b/src/main.rs
index d23e029..7b2e45c 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -514,17 +514,18 @@ fn main() {
         // https://github.com/tldr-pages/tldr/blob/main/CLIENT-SPECIFICATION.md#page-names
         let command = args.command.join("-").to_lowercase();
 
+        // Collect languages
         let languages = args
             .language
             .map_or_else(get_languages_from_env, |lang| vec![lang]);
 
         // Search for command in cache
-        if let Some(page) = cache.find_page(
+        if let Some(lookup_result) = cache.find_page(
             &command,
             &languages,
             config.directories.custom_pages_dir.as_deref(),
         ) {
-            if let Err(msg) = print_page(&page, args.raw, &config) {
+            if let Err(msg) = print_page(&lookup_result, args.raw, &config) {
                 print_error(enable_styles, &msg);
                 process::exit(1);
             }
diff --git a/src/output.rs b/src/output.rs
index 70cd138..cad0690 100644
--- a/src/output.rs
+++ b/src/output.rs
@@ -1,9 +1,6 @@
 //! Functions for printing pages to the terminal
 
-use std::{
-    fs::File,
-    io::{self, BufRead, BufReader, Write},
-};
+use std::io::{self, BufRead, Write};
 
 use crate::{
     cache::PageLookupResult,
@@ -15,41 +12,44 @@ use crate::{
 
 /// Print page by path
 pub fn print_page(
-    page: &PageLookupResult,
+    lookup_result: &PageLookupResult,
     enable_markdown: bool,
     config: &Config,
 ) -> Result<(), String> {
+    // Create reader from file(s)
+    let reader = lookup_result.reader()?;
+
+    // Lock stdout only once, this improves performance considerably
     let stdout = io::stdout();
     let mut handle = stdout.lock();
 
-    for path in page.paths() {
-        let file = File::open(path).map_err(|msg| format!("Could not open file: {}", msg))?;
-        let reader = BufReader::new(file);
-
-        if enable_markdown {
-            // Print the raw markdown of the file.
-            for line in reader.lines() {
-                writeln!(handle, "{}", line.unwrap())
-                    .map_err(|_| "Could not write to stdout".to_string())?;
+    if enable_markdown {
+        // Print the raw markdown of the file.
+        for line in reader.lines() {
+            writeln!(handle, "{}", line.unwrap())
+                .map_err(|_| "Could not write to stdout".to_string())?;
+        }
+    } else {
+        // Closure that processes a page snippet and writes it to stdout
+        let mut process_snippet = |snip: PageSnippet<'_>| {
+            if snip.is_empty() {
+                Ok(())
+            } else {
+                print_snippet(&mut handle, snip, &config.style)
+                    .map_err(|e| WriteError(e.to_string()))
             }
-        } else {
-            let mut process_snippet = |snip: PageSnippet<'_>| {
-                if snip.is_empty() {
-                    Ok(())
-                } else {
-                    print_snippet(&mut handle, snip, &config.style)
-                        .map_err(|e| WriteError(e.to_string()))
-                }
-            };
-            highlight_lines(
-                LineIterator::new(reader),
-                &mut process_snippet,
-                !config.display.compact,
-            )
-            .map_err(|e| format!("Could not write to stdout: {}", e.message()))?;
         };
-    }
 
+        // Print highlighted lines
+        highlight_lines(
+            LineIterator::new(reader),
+            &mut process_snippet,
+            !config.display.compact,
+        )
+        .map_err(|e| format!("Could not write to stdout: {}", e.message()))?;
+    };
+
+    // We're done outputting data, flush stdout now!
     handle
         .flush()
         .map_err(|_| "Could not flush stdout".to_string())?;


Test patch:
diff --git a/tests/inkscape-v2.patch b/tests/inkscape-v2.patch
index ffca34c..5cc5d22 100644
--- a/tests/inkscape-v2.patch
+++ b/tests/inkscape-v2.patch
@@ -1,5 +1,3 @@
-This header shouldn't be required
-=================================
 Custom inkscape entry
 
     My Inkscape example