Auto updating local data if too old

[riesentoaster]

What does it do?

If the local data is too old, it now no longer asks you to manually update it but attempts to automatically do so. Regardless of whether it succeeds, the output is shown.

Why the change?

It is annoying to manually update it. See also #81.

How can this be tested?

Change the date in ~/.tldrc/date to something further back than the past two weeks, run any tldr command like tldr tldr

Where to start code review?

Very few lines changed in local.c

Relevant tickets?

Closes #81.

Questions?

This is just a first attempt. If you'd like me to change how this is implemented, I will gladly do that. I just wanted to start the conversation.

[riesentoaster]

Would one of the maintainers care to take a look at this? Pinging @MasterOdin, @owenvoke, whoever else wants do.

[github-actions]

Hi all! This thread has not had any recent activity.
Are there any updates? Thanks!

[github-actions]

Hi all! This thread has not had any recent activity.
Are there any updates? Thanks!

[riesentoaster]

Once again pinging @MasterOdin, @owenvoke, @kbdharun or any other maintainer. Any feedback on this?

[kbdharun]

Once again pinging <> maintainer. Any feedback on this?

Hi, I too think this will be useful, unfortunately, I can't review this extensively as I am not proficient with C even though I know it a bit. I will skim through it, but I would suggest other maintainers reviewing it.

[SethFalco]

Change makes sense to me, but just some UX (color) concerns!

[SethFalco]

Actually, also want to nitpick at the copy.

No need for the word automatic here, the user knows it's automatic because it was automatically done for them!

It's also a bit jarring to refer to it as data in one string, and then database immediately after. ^-^'
It's referred to as local database explicitly everywhere else, let's stick with that!

See how the only instances of local data (without having base affixed to it) are the instances from this PR.

[SethFalco]

I'm so sorry for the review spam, I just keep getting new thoughts when I think I'm finished! I think this will be a no-go as it is now.

There are users who specifically don't want it to automatically try to update the cache, i.e. due to poor network conditions.

• See Do not invoke update_localdb if provide with an option flag. #27

Having logic to preserve the original behavior as default, and only perform the update when toggled, i.e. via environment variable or client setting should be a prerequisite.

I think this could be easily/simply implemented by giving users the option to set an environment variable, e.g. TLDR_AUTOUPDATE.

— #81

[riesentoaster]

Thank you so much for looking at it!

I agree with both the color and wording points, those seem reasonable. For your last point: That is a bit of an issue, can't have it both ways. The current archive is ~6MB.

My personal preference would be to automatically download the updates and having an option with either an argument and/or an environment variable to disable it. If I am currently in an environment where the network is slow enough for me to care about it, I'd probably like one of the following behaviours:

1. If the automatic download takes more than say 3 seconds, it is cancelled, and the output is displayed anyway along with a message that tells the user to manually update the database.
2. The download is started and a message is displayed that tells the user what exact command they'd have to use to prevent the automatic update, so when they get annoyed, they could just kill the process (ctrl-c) and copy paste the line to instantly get the output.
3. The old version is displayed automatically and the download is only started afterwards. If there was an update to the explanation of this specific command, the new version is printed (and potentially the old one deleted). This way, the user immediately gets an answer and can continue working while the download still is in progress. If they want to use the same terminal immediately, they can either kill the process or move it to the background.

I only ever invoke tldr manually and I'm not sure when you'd ever do so in a script, so having a manual cancellation option seems ok to me.

Opinions on this?

[SethFalco]

My personal preference would be to automatically download the updates and having an option with either an argument and/or an environment variable to disable it.

Overall, this is actually my preference as well. If this were a new client, I'd expect to see this.

However, when a project is already out and being used in the wild, I think it's often best to make changes like these iteratively. So to preserve the original behavior and make the option to opt-in for automatic updates, and at a later time change the default if it's deemed safe to do so. However, I'm happy to concede if other maintainers think it's not a big deal.

If the automatic download takes more than say 3 seconds, it is cancelled, and the output is displayed anyway along with a message that tells the user to manually update the database.

This wouldn't be good UX. No one likes inconsistent behavior. An implementation like this leads to unpredictable output since it entirely depends on network conditions. Even on a fast network, sometimes it can be congested.

The download is started and a message is displayed that tells the user what exact command they'd have to use to prevent the automatic update

This is viable. 👍

The old version is displayed automatically and the download is only started afterwards. If there was an update to the explanation of this specific command, the new version is printed (and potentially the old one deleted).

This wouldn't be a good UX. It'd be like the terminal equivalent of CLS (Cumulative Layout Shift) in browsers. At that point, I think it'd be better to just make users wait for the update. Imo this is more work, for a lesser experience.

---

From my perspective, so long as it's configurable (opt-in or opt-out), I'd be willing to approve the PR. But if the default behavior changes, I'll wait for another maintainer to approve/merge before merging it myself, just to solidify if we're happy to change it.

PS: While I had some negative feedback for some of the ideas, I do like and appreciate that you pitched numerous designs for this. 👍 Don't be afraid to tell me if you think I'm wrong about anything!

[riesentoaster]

Thank you for your feedback! I'm just pitching ideas and as long as we end up agreeing on something, I am happy.

So, assuming we implement a --prevent-update flag: What do we do with the --update flag?

• Removing it would introduce more breaking changes (but one could also argue that since we're already touching it, might as well…).
• Not removing it leads to a weird situation when both flags are used.
  • Do we just print an error message?
  • Or give one priority and print an error message as well?
  • Or just give one priority and ignore the other?
• Otherwise I'd say that using no flags would just trigger the auto update.
• Using just the --prevent-update flag would output the page from the local database. (Would we even check for the age and print a message if it is too old then? Since the user specifically asked not to update it?)
• Using just the --update flag would have the same behaviour as it has now: Update the database and don't print any other output.

[SethFalco]

Hmm, I don't think a flag would be ideal for this. It's not like users will use --prevent-update every single time they use the client. ^-^'

Removing the --update arg isn't an option, all clients with a cache support -u / --update, this is also defined in the client specifications. Even if it's redundant, it should stay.

Assuming the default behavior changes to auto-update, users can just set an environment variable like TLDR_AUTO_UPDATE_DISABLED=1 if they don't want the cache to update automatically.

Think of it like DOTNET_CLI_TELEMETRY_OPTOUT or NUXT_TELEMETRY_DISABLED.

This is basically the inverse of the original proposal to have an environment variable to enable auto-updates.

If the environment variable is defined, auto-updates are not performed unless the --update flag is present. For such users, when the local database is > 2 weeks old, or a command doesn't exist, it will not update unless --update is present.

[riesentoaster]

That seems like an elegant solution. I implemented it, added a comment to the message explaining that the auto update is about to happen and one to the README for documentation.

I also added a check that prevents tldr from doing its online lookup in case a requested page isn't available locally, as you suggested.

With those changes this PR would be ready for a new round of reviews in my opinion, so if anyone would like to take a look, please do!

[SethFalco]

Overall, this looks pretty good to me, thanks for taking the time to make implement this. Once the issues I noted are resolved, I'd be happy to approve this.

We'll need other maintainers to review the UX regarding prints specific for disabled automatic updates, but I'll request more reviews once everything else is fine. 🤔

[SethFalco]

Almost there, one petty change and I think this is good.
I hope other maintainers agree. ^-^

[SethFalco]

I think this is a good change. 👍
Thank you for your contribution!

I'm happy to approve, but I would prefer to wait for an OG maintainer to chime and confirm they are happy with the default behavior changing before we merge.

I'll pester people in Matrix if no one wants to come around to review this soon.

Edit: I think this also solves #27, so after this, I'll also close that and post a comment with how to achieve the desired behavior.

[kbdharun]

LGTM, Thanks for your contribution.

[riesentoaster]

I'll pester people in Matrix if no one wants to come around to review this soon.

@SethFalco Any progress on this?

[MasterOdin]

Thanks for your contribution. I think it may make sense to release this as part of a major release to better signify the change in behavior around when updates get downloaded.

[kbdharun]

Thanks for your contribution. I think it may make sense to release this as part of a major release to better signify the change in behavior around when updates get downloaded.

Offtopic: I think we should create one for our Python and Elixr client too, as we updated the tldr source from master to main (+ we were planning to deprecate the master branch next month)