tealdeer-rs / tealdeer

A very fast implementation of tldr in Rust.
https://tealdeer-rs.github.io/tealdeer/
Apache License 2.0
4.17k stars 123 forks source link

Add `no-auto-update` flag #257

Closed cyqsimon closed 2 years ago

cyqsimon commented 2 years ago

I will need some help with the completion scripts since I have no experience with these.

cyqsimon commented 2 years ago

Also I think the Args struct for clap could use some cleanup. More guard clauses in main() can be accomplished with requries and conflicts_with using clap, for example. But that belongs in a separate PR.

niklasmohrin commented 2 years ago

Rustfmt change makes sense to me, good catch!

About the auto update: Auto update is a convenience feature and having to specify this flag is inconvenient. What if we only warn if auto updates fail because one is offline? @dbrgn

cyqsimon commented 2 years ago

Auto update is a convenience feature and having to specify this flag is inconvenient. What if we only warn if auto updates fail because one is offline?

I'm not particularly convinced that automatic detection of network availability/condition is a full solution, mostly for the reason that oftentimes network failure/non-availability manifests in the form of a timeout after an extensive wait. Imagine being on a train in a remote region with questionable cellular coverage and using your mobile hotspot for internet - it's pretty likely that the update doesn't fail immediately but suffers from terrible speed and frequent retries. Truth be told, this is in fact my exact situation a few days ago.

That being said I do agree that if auto update fails, it's better to make it a warning rather than a hard error. So this change would be a welcomed one in my books - I just don't think it solves the problem fully. I would insist that there needs to be a manual "off switch" for situations where the network is not completely dead but pretty unreliable. At least having the option of typing an additional flag and getting a response reliably is better than having to wait out a network timeout, or worse, not being able to use the client at all.

niklasmohrin commented 2 years ago

Okay, I can see your point. If we implement this, I would make the setting more generic, like "offline" or so. I did a quick search whether there is a convention for this kind of setting, but I couldn't find one. If there would be some (linux) convention that, let's say the OFFLINE env variable, is set for this scenario, I would be totally in favor of respecting that (but not adding tealdeer specific settings). With an extra flag, say --offline, I feel like this is a big change for a rather niche usecase. I am not strictly against it, it's just a lot of work that is expected from the program and it involves this system of settings that turn things on and off in different places; after all, tealdeer is offline by default

dbrgn commented 2 years ago

The empty rustfmt.toml file is a good idea indeed!

To keep this discussion focussed on the no-auto-update flag, I'll cherry-pick that commit onto the main branch.

dbrgn commented 2 years ago

I don't think there's a generic "offline"-variable, or at least I've never heard of such a thing. It would also open up a lot of questions, e.g. what happens if you run OFFLINE=1 tldr --update?

I think having a --no-auto-update file is fine, it's a simple change and I don't expect that it'll add more complexity in the future.

cyqsimon commented 2 years ago

I'm not very familiar with this workflow of cherry picking commits in the middle of a PR. Hopefully I haven't managed to screw everything up.

Also as a reminder the shell completion scripts haven't been updated yet.

cyqsimon commented 2 years ago

I updated the completion scripts but I haven't tested them. As aforementioned, I do not have experience working with those so I might be making beginner mistakes. Please review these changes rather cautiously.

dbrgn commented 2 years ago

Ahh, now the branch conflicts with the pull request #259 that was merged just recently 🙈

@cyqsimon if you want to update the pull request yourself:

# Only if you don't already have a remote...
git remote add upstream https://github.com/dbrgn/tealdeer/

# Update upstream source
git fetch upstream

# Ensure you're on the no-auto-update branch
git checkout no-auto-update

# Rebase and fix conflicts
git rebase upstream/main

If you want us to merge or rebase your commits for you, that's no problem either, just let us know!

cyqsimon commented 2 years ago

Done