Support custom pages and patches #142

dmaahs2017 · 2020-09-22T21:50:27Z

Feature implementation for issue #98
This is a feature that lets you create custom tldr pages locally.
You can set a directory where tealdeer will search for these local pages with the env variable TEALDEER_CUSTOM_PAGES_DIR. If this is not set then it will use the tldr-master/pages.custom

If you give a custom tldr page the same name as an existing one, then your custom one will be appended to it.

Example with existing tldr page:

Example with no existing tldr page:

I find the value added here is that overtime a person can build their own collection of common use-cases to a particular command that they use semi-frequently. I created this because I would create my own repository of command specific use-cases, and to document personalized scripts for myself.

Side note: This is my first real open-source contribution, and I'm not super familiar w/ the ins and outs of rust. Please leave suggestions for improvements 😄 .

Update, Dec 18. Implemented suggested changes from @dbrgn

Implemented .page and .patch differences. {}.page files overwrite existing tldr entries. {}.patch appends to existing entries.
Now using the config file for custom_pages_dir, default directory is CONFIGDIR/pages
Also added 2 flags, --patch and --overwrite. These commands create and or open the .page or .patch files in your $EDITOR. I added this because I found it was annoying to constantly switch the the custom page directory. Also this enables people to very quickly make changes to their custom patches/pages

Example of a .patch file in work

Example of a new tldr page with a .page file

Example of overwriting an existing tldr page using a .page file

How the config looks now

if the .md file is the same as one that already exists then the custom one gets appended to the original one Added TEALDEER_CUSTOM_PAGES_DIR env variable If set then tealdeer will search that directory for custom pages otherwise it will search "pages.custom"

src/cache.rs

dvergeylen · 2020-11-20T10:03:53Z

Hey,

This looks very promising, is there any plan to merge anytime soon? 😇

Keep up the great work! 💯

dmaahs2017 · 2020-11-26T04:20:54Z

Hey,

This looks very promising, it there any plan to merge it anytime soon? 😇

Keep up the great work! 💯

Great to hear back from you @dvergeylen! 😊

I was just waiting on a code review, since I'm fairly new to Rust. I'm happy to make changes

But if you don't have any suggestions then it should be good to merge

dbrgn

Hi @dmaahs2017, sorry for the long delay, especially since this is your first open source contribution. I'm currently quite busy at work and with other projects, so that leaves less time for maintaining tealdeer 🙂

I left two comments!

src/cache.rs

.page files overwrite any and all other files. .patch files append to prexisting pages.

default is ~/.local/share/tealdeer_custom_pages, if HOME env var is not set, then it defaults to "".

next fallbacks are on ~/.local/share/custom_tldr_pages/ then /.local/share/custom_tldr_pages/ if no $HOME variable exists

`tldr --patch` to create/edit a new patch file. `tldr --overwrite` to create/edit a new page file. The logic of this is simply: use the $EDITOR variable to initiate an editor on CUSTOM_PAGES_DIR/page.{patch,page}, exentension is determined by which flag you pass. It also checks that CUSTOM_PAGES_DIR exists and creates it if it does not.

…dr_pages # Conflicts: # src/main.rs

niklasmohrin

Looks good overall, thank you! I have some suggestions that mostly involve moving code around and renaming things to better fit (my perception of) what is actually happening. I also suggested something about your last comment, but I am not sure it's the best thing to do and I would want to hear what @dbrgn thinks

src/cache.rs

src/config.rs

src/main.rs

src/cache.rs

niklasmohrin · 2021-02-03T16:52:04Z

Oh, and I think that if a patch and a custom page exist, both should be output, forgot to mention that. I don't think that it would be a great pain, because I would suggest looking for patches once in find_pages (or another outer function) and appending it to whatever page is found.

dbrgn · 2021-02-03T22:57:00Z

Oh, and I think that if a patch and a custom page exist, both should be output, forgot to mention that.

Hm, I'm not sure if I agree. I can't really think of a use case where a user would add both a custom page file and a patch file. I think I'd stick to rendering just the page, unless there's a compelling reason to apply the patch as well. As @dmaahs2017 wrote, the implementation with an early return for custom pages is more efficient, so I'd prefer that.

Secondly, I'm not sure how to handle this? Is this non-recoverable should it panic?

~~That's a good question. Maybe the custom_pages_dir config could be an Option? If it cannot be determined, we could simply skip the "custom patch" logic.~~

Another option would be to propagate the error, but that's not possible in a Default implementation. And a panic is also not the nicest thing from a user point of view (although it might be appropriate if the app directory cannot be determined, this should not happen). @niklasmohrin what do you think?

(Edit: Ah, @niklasmohrin already commented on that above 😄)

dmaahs2017 · 2021-02-04T09:57:17Z

@niklasmohrin and @dbrgn love the feedback. I'm learning a ton working on this 😊.

Expect me to address the new reviews by Friday evening

* Changed config for custom_dir to be an Option * Some refactoring and renaming * Moved the looping logic over PageLookupResult to be in the print_page function * Moved finding custom patch call into the find_page function.

niklasmohrin

Wow, you were a lot quicker than us 😅 I have added some small comments that should give this the final touch. Please also make sure to check CI (red cross next to your commit hash) when you push, clippy usually has some pretty nice suggestions too (it's insane what kind of stuff clippy can detect).

Other than that, great stuff! Keep it up 🚀

src/cache.rs

dbrgn · 2021-02-04T23:34:11Z

Please also make sure to check CI (red cross next to your commit hash) when you push, clippy usually has some pretty nice suggestions too (it's insane what kind of stuff clippy can detect).

Note: I just bumped Rust and Clippy to 1.41 on master, so to successfully pass the CI you'll need to rebase your branch (or merge master into it).

You can also run clippy locally: cargo clean && cargo +1.41.1 clippy.

* Revert find_page_for_platform back to return PathBuf * find_page no longer takes the entire config as an argument * various other refactoring

dmaahs2017 · 2021-02-05T01:49:15Z

@dbrgn

You can also run clippy locally: cargo clean && cargo +1.41.1 clippy.

I'm not sure why, but I'm on the stable toolchain (and I merged master)

Edit: Looks like it passed the continuous integration anyways 😄

niklasmohrin

Sweet!

Yeah, I totally love these shorthand methods on Option and Result too, I think it absolutely worth it to scroll through the officials docs to see every one of them. I also saw this reddit post a while back.

Finally, the +1.41 syntax for cargo means that the 1.41 toolchain from rustup should be used instead of the default (probably stable). For that to work, you have to have the 1.41 toolchain installed though (rustup toolchain install 1.41). This only works if you have installed cargo through rustup (on Arch, you could install rustc and cargo alone; then this would not work). As you said, CI passes so that is a fine for this PR :)

niklasmohrin · 2021-02-05T09:59:34Z

(note to self and @dbrgn: don't forget to squash these 16 commits)

niklasmohrin · 2021-03-17T11:15:28Z

Hey @dmaahs2017, casual reminder that we have not forgotten your PR 👍

@dbrgn Any chance we can merge this some time soon?

dbrgn

Thanks very much for your patience, @dmaahs2017 (and thanks @niklasmohrin for the reviews) 🙂 I really like how this PR turned out, especially the PageLookupResult!

There are only three minor nitpicks left, and I'll merge it as soon as those are fixed!

We should also include the custom pages directory in the new --show-paths option output, but that's probably best done in a separate PR, in order not to hold up this one.

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.

src/cache.rs

- Removed dev comment - rename maybe_config_dir -> custom_pages_dir

niklasmohrin

Still great! I only now noticed that there are no tests for the new behavior, but I would honestly rather have this PR merged and have the tests in another PR

dmaahs2017 · 2021-04-22T14:01:13Z

Still great! I only now noticed that there are no tests for the new behavior, but I would honestly rather have this PR merged and have the tests in another PR

Oh yeah! I'll create myself an issue for that :)

dmaahs2017 force-pushed the custom_tldr_pages branch from 1910018 to fc81872 Compare September 22, 2020 22:01

dmaahs2017 marked this pull request as ready for review September 22, 2020 22:09

dmaahs2017 force-pushed the custom_tldr_pages branch 3 times, most recently from 208d659 to 99277ff Compare September 22, 2020 23:28

dmaahs2017 changed the title ~~Custom tldr pages in tldr-master/pages.custom~~ Support custom pages in a custom directory or in tldr-master/pages.custom Sep 22, 2020

dmaahs2017 force-pushed the custom_tldr_pages branch 2 times, most recently from c170d88 to c7233c7 Compare September 22, 2020 23:36

dmaahs2017 force-pushed the custom_tldr_pages branch from c7233c7 to e135101 Compare September 22, 2020 23:39

dmaahs2017 commented Sep 23, 2020

View reviewed changes

src/cache.rs Outdated Show resolved Hide resolved

dmaahs2017 marked this pull request as draft September 23, 2020 15:31

dmaahs2017 mentioned this pull request Sep 23, 2020

Allow adding custom entries to pages #98

Closed

dmaahs2017 marked this pull request as ready for review September 24, 2020 01:26

dmaahs2017 force-pushed the custom_tldr_pages branch from 2ade1d1 to 4c0ac7d Compare September 24, 2020 01:32

dmaahs2017 marked this pull request as draft September 24, 2020 01:37

Fix bug where common path over-rides platform path

b5ab5cb

dmaahs2017 force-pushed the custom_tldr_pages branch from 4c0ac7d to b5ab5cb Compare September 24, 2020 02:15

dmaahs2017 marked this pull request as ready for review September 24, 2020 02:16

dbrgn requested changes Nov 26, 2020

View reviewed changes

src/cache.rs Outdated Show resolved Hide resolved

src/cache.rs Outdated Show resolved Hide resolved

dmaahs2017 added 2 commits December 18, 2020 01:35

use .page and .patch for custom pages.

e9e8c4b

.page files overwrite any and all other files. .patch files append to prexisting pages.

Use config file for custom_pages_dir

2261105

default is ~/.local/share/tealdeer_custom_pages, if HOME env var is not set, then it defaults to "".

dmaahs2017 force-pushed the custom_tldr_pages branch from 1fa9bc6 to 2261105 Compare December 18, 2020 08:33

dmaahs2017 requested a review from dbrgn December 18, 2020 08:44

dmaahs2017 added 4 commits December 18, 2020 14:43

change default custom_pages_dir to CONFIGDIR/pages

da6e784

next fallbacks are on ~/.local/share/custom_tldr_pages/ then /.local/share/custom_tldr_pages/ if no $HOME variable exists

Merge remote-tracking branch 'remotes/upstream/master' into custom_tl…

8ff8ce3

…dr_pages # Conflicts: # src/main.rs

update usage.docopt

fffd03b

niklasmohrin requested changes Feb 3, 2021

View reviewed changes

3rd round review changes

a468315

* Changed config for custom_dir to be an Option * Some refactoring and renaming * Moved the looping logic over PageLookupResult to be in the print_page function * Moved finding custom patch call into the find_page function.

dmaahs2017 requested review from dbrgn and niklasmohrin February 4, 2021 18:49

niklasmohrin requested changes Feb 4, 2021

View reviewed changes

src/cache.rs Outdated Show resolved Hide resolved

src/cache.rs Outdated Show resolved Hide resolved

src/cache.rs Outdated Show resolved Hide resolved

src/cache.rs Show resolved Hide resolved

src/cache.rs Outdated Show resolved Hide resolved

dmaahs2017 added 2 commits February 4, 2021 18:51

Merge remote-tracking branch 'upstream/master' into custom_tldr_pages

ea79c84

Review Round 4

3da3227

* Revert find_page_for_platform back to return PathBuf * find_page no longer takes the entire config as an argument * various other refactoring

dmaahs2017 requested a review from niklasmohrin February 5, 2021 01:53

niklasmohrin approved these changes Feb 5, 2021

View reviewed changes

dbrgn requested changes Apr 14, 2021

View reviewed changes

src/cache.rs Outdated Show resolved Hide resolved

src/cache.rs Outdated Show resolved Hide resolved

src/cache.rs Outdated Show resolved Hide resolved

dmaahs2017 force-pushed the custom_tldr_pages branch from 445c5dd to 2827fb5 Compare April 14, 2021 22:50

Address Change Requests

704234e

- Removed dev comment - rename maybe_config_dir -> custom_pages_dir

dmaahs2017 force-pushed the custom_tldr_pages branch from 2827fb5 to 704234e Compare April 14, 2021 22:50

dmaahs2017 requested review from dbrgn and niklasmohrin April 18, 2021 02:05

niklasmohrin approved these changes Apr 18, 2021

View reviewed changes

dbrgn approved these changes Apr 18, 2021

View reviewed changes

dbrgn merged commit 07c7156 into tealdeer-rs:master Apr 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support custom pages and patches #142

Support custom pages and patches #142

dmaahs2017 commented Sep 22, 2020 •

edited

Loading

dvergeylen commented Nov 20, 2020 •

edited

Loading

dmaahs2017 commented Nov 26, 2020 •

edited

Loading

dbrgn left a comment

niklasmohrin left a comment

niklasmohrin commented Feb 3, 2021

dbrgn commented Feb 3, 2021 •

edited

Loading

dmaahs2017 commented Feb 4, 2021 •

edited

Loading

niklasmohrin left a comment

dbrgn commented Feb 4, 2021

dmaahs2017 commented Feb 5, 2021 •

edited

Loading

niklasmohrin left a comment

niklasmohrin commented Feb 5, 2021

niklasmohrin commented Mar 17, 2021

dbrgn left a comment

niklasmohrin left a comment

dmaahs2017 commented Apr 22, 2021

Support custom pages and patches #142

Support custom pages and patches #142

Conversation

dmaahs2017 commented Sep 22, 2020 • edited Loading

Update, Dec 18. Implemented suggested changes from @dbrgn

dvergeylen commented Nov 20, 2020 • edited Loading

dmaahs2017 commented Nov 26, 2020 • edited Loading

dbrgn left a comment

Choose a reason for hiding this comment

niklasmohrin left a comment

Choose a reason for hiding this comment

niklasmohrin commented Feb 3, 2021

dbrgn commented Feb 3, 2021 • edited Loading

dmaahs2017 commented Feb 4, 2021 • edited Loading

niklasmohrin left a comment

Choose a reason for hiding this comment

dbrgn commented Feb 4, 2021

dmaahs2017 commented Feb 5, 2021 • edited Loading

niklasmohrin left a comment

Choose a reason for hiding this comment

niklasmohrin commented Feb 5, 2021

niklasmohrin commented Mar 17, 2021

dbrgn left a comment

Choose a reason for hiding this comment

niklasmohrin left a comment

Choose a reason for hiding this comment

dmaahs2017 commented Apr 22, 2021

dmaahs2017 commented Sep 22, 2020 •

edited

Loading

dvergeylen commented Nov 20, 2020 •

edited

Loading

dmaahs2017 commented Nov 26, 2020 •

edited

Loading

dbrgn commented Feb 3, 2021 •

edited

Loading

dmaahs2017 commented Feb 4, 2021 •

edited

Loading

dmaahs2017 commented Feb 5, 2021 •

edited

Loading