Skip to content

Conversation

@kahlstrm
Copy link
Contributor

@kahlstrm kahlstrm commented Apr 6, 2025

First of all, thanks for this wonderful piece of software. I Have been enjoying it for over a year!

However, one particular caveat I have encountered is when I run tldr after a
a couple of weeks to remind me of e.g. how to use tar (that already
exists in the cache), I first have to wait to download a cache update prior
I want to see the result already in my local (but stale) cache.

Alternatively, I could use --offline, but then I would need to run tldr --update manually sometimes,
I would need to run the command twice for new commands, first with --offline and then without.

This PR tries to address this by adding a --optimistic-cache-flag that, when enabled, adds the following cases:

  • If the cache was found but is stale, it does the lookup first against the stale cache and prints out the result if it is found.
    After displaying the result, it runs the update and also communicates to the user if the cache is out of date.
  • If the entry is not in the stale cache, we will re-try the lookup by updating the stale cache first.

To avoid doing massive amounts of refactoring, I had to do a bit of hack by first
separating CLI and config parsing from the run-function, and then in the optimistic cache
the case where the page is not found, call run recursively by disabling the optimistic cache flag.

Let me know if

  • This feature is unwanted, and I should use --offline instead.
  • The proposed solution is too hacky, and you want me to refactor/do more code-splitting for the PR instead.

@acuteenvy
Copy link
Member

Alternatively, I could use --offline, but then I would need to run tldr --update manually sometimes

If that delay annoys you, then you can always set up something to run tldr --update for you automatically.
Here's an example using systemd:

[Unit]
Description=Update the cache of tldr pages
After=network.target

[Service]
Type=oneshot
ExecStart=tldr --update

[Install]
WantedBy=default.target

Put that in ~/.config/systemd/user/tldr-update.service and run systemctl --user enable tldr-update. The cache will be updated on every boot when you log in as your user, in the background. You can then set auto_update = false in the tlrc config, since it doesn't make sense to have automatic updates before viewing a page with this configuration.


As for this PR, yeah the recursive run() hack is something I'd like to avoid, but that can be refactored quite easily. I'm more concerned about whether this feature is useful. The delay we're talking about is a couple seconds, every two weeks (and that can be changed already if you'd like to update less often). On the off chance that this does irk someone, you can always run updates in the background.

Let's get some more opinions though, I'll wait for other maintainers.

@kahlstrm
Copy link
Contributor Author

kahlstrm commented Apr 7, 2025

Alternatively, I could use --offline, but then I would need to run tldr --update manually sometimes

If that delay annoys you, then you can always set up something to run tldr --update for you automatically. Here's an example using systemd:

[Unit]
Description=Update the cache of tldr pages
After=network.target

[Service]
Type=oneshot
ExecStart=tldr --update

[Install]
WantedBy=default.target

Put that in ~/.config/systemd/user/tldr-update.service and run systemctl --user enable tldr-update. The cache will be updated on every boot when you log in as your user, in the background. You can then set auto_update = false in the tlrc config, since it doesn't make sense to have automatic updates before viewing a page with this configuration.

As for this PR, yeah the recursive run() hack is something I'd like to avoid, but that can be refactored quite easily. I'm more concerned about whether this feature is useful. The delay we're talking about is a couple seconds, every two weeks (and that can be changed already if you'd like to update less often). On the off chance that this does irk someone, you can always run updates in the background.

Let's get some more opinions though, I'll wait for other maintainers.

Yeah, found out about tldr-update-service in Home Manager right after filing this PR, and that seems to do similar things as that systemd-configuration. The only problem for me is that it currently does not support macOS that I use at work. But yeah, I guess I could write a manual implementation of that systemd service for macOS as well, and that would do it 👍.

Nevertheless, if this is deemed worthwhile, I can do the work to remove the hacky recursiveness, but I'll wait for confirmation before doing that if this is a wanted feature or not.

@devnoname120
Copy link

devnoname120 commented Apr 9, 2025

@acuteenvy Hmm for macOS and Windows it would be a different syntax, not ideal when managing one's dotfiles. I also find it a bit of an annoyance. IMO the cache update should happen after the command or in parallel, not before because it means having to wait a bit which can interrupt the flow. It's not a huge thing, just a little thing that I think would be nice.

@gsspdev
Copy link

gsspdev commented May 3, 2025

@acuteenvy Commenting to say that I also find waiting for tldr to update sufficiently annoying and if you're not someone that's already a regular tldr user having to wait for it to update when you do decide to try out can be enough to turn a user off from making tldr their go to quick command info finder

tldr should feel fast to run or users everytime imo. speed is the point of using something like tldr. hope this gets implemented

@acuteenvy acuteenvy self-requested a review May 5, 2025 17:41
@kahlstrm
Copy link
Contributor Author

kahlstrm commented May 7, 2025

@acuteenvy made some progress on cleaning this up, but I got lost in doing other things to separate the logic from the ' run `function. The optimistic cache update is now running during the drop of the cache, provided it was deemed necessary, and only if it is still necessary to run it.

Marked commit as wip as I would like to write some tests for this, but I'm unsure when I have the time.

@kahlstrm kahlstrm force-pushed the main branch 3 times, most recently from 7ee4557 to 1b05f86 Compare May 7, 2025 04:52
Copy link
Member

@acuteenvy acuteenvy left a comment

Choose a reason for hiding this comment

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

Alright, since some more people said this feature is useful, I think we can implement it. I have a couple suggestions though:

  1. I think having this in the config is enough. This feature isn't something that a user will frequently change, so a command-line option isn't necessary.
  2. impl Drop for Cache isn't a very good place to put page updating code into, because you can't return errors from it.
  3. The name optimistic_cache doesn't really explain what this option does. How about defer_auto_update?

I can try fixing the original recursive run workaround and the issues I've pointed out if you want to.

@kahlstrm
Copy link
Contributor Author

kahlstrm commented May 9, 2025

Alright, since some more people said this feature is useful, I think we can implement it. I have a couple suggestions though:

  1. I think having this in the config is enough. This feature isn't something that a user will frequently change, so a command-line option isn't necessary.
  2. impl Drop for Cache isn't a very good place to put page updating code into, because you can't return errors from it.
  3. The name optimistic_cache doesn't really explain what this option does. How about defer_auto_update?

I can try fixing the original recursive run workaround and the issues I've pointed out if you want to.

  1. Good point, removed and only made it a config-only option 👍
  2. Moved this to fn check_deferred_auto_update
  3. Renamed to defer_auto_update 👍

Let me know if there is something that you'd like to see added/changed, or feel free to make edits/fix the original version as well, I'm fine with any option as long as this feature gets added 👍

@kahlstrm kahlstrm changed the title Add support for optimistic cache functionality, separate cli and config parsing from run Add support for deferring auto update after page render May 9, 2025
@acuteenvy
Copy link
Member

I tried to solve the issues of your original version. I think it works as intended, could you test whether it works for you?

@kahlstrm
Copy link
Contributor Author

I tried to solve the issues of your original version. I think it works as intended, could you test whether it works for you?

Hey, tested it and yeah, this one works as intended 👍 thanks for fixing it!

This should be re-added after a new release
@acuteenvy
Copy link
Member

Thank you!

@acuteenvy acuteenvy merged commit 94236cc into tldr-pages:main May 14, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants