Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add no-auto-update flag #257

Merged
merged 4 commits into from
Jan 21, 2022
Merged

Add no-auto-update flag #257

merged 4 commits into from
Jan 21, 2022

Conversation

cyqsimon
Copy link
Contributor

  • Adding an empty rustfmt.toml enforces default formatter settings regardless of user's global settings
  • Currently if the user enables auto update in the config file, there is no way to temporarily disable it via CLI
    • This is a problem when for example, the network is not available or is very slow. The client simply errors and refuses to display anything, despite the fact that the cache may be perfectly good (albeit a little old)
    • Adding no-auto-update flag allows the client to stay usable in such situations

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

@cyqsimon
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

cyqsimon commented Jan 16, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

dbrgn commented Jan 16, 2022

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 pushed a commit that referenced this pull request Jan 16, 2022
@dbrgn dbrgn changed the title Added empty rustfmt.toml and added no-auto-update flag Add no-auto-update flag Jan 16, 2022
@dbrgn
Copy link
Collaborator

dbrgn commented Jan 16, 2022

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.

Copy link
Collaborator

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

@cyqsimon can you rebase your branch against main and also update the usage docs with the changed ?

cargo run -- --help > docs/src/usage.txt

@dbrgn dbrgn added enhancement waiting-for-author The PR needs an update before it can be considered for merging. labels Jan 16, 2022
@cyqsimon
Copy link
Contributor Author

cyqsimon commented Jan 17, 2022

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 cyqsimon requested a review from dbrgn January 17, 2022 05:10
@cyqsimon
Copy link
Contributor Author

cyqsimon commented Jan 17, 2022

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.

fish_tealdeer Outdated Show resolved Hide resolved
Copy link
Collaborator

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

LGTM now. @niklasmohrin do you agree?

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.

Everything looks correct 🙂 If you have any future questions regarding git, don't hesitate to ask, we're happy to help.

fish_tealdeer Outdated Show resolved Hide resolved
Copy link
Collaborator

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Code change looks good, I want to discuss some texts though :^)

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@dbrgn
Copy link
Collaborator

dbrgn commented Jan 20, 2022

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
Copy link
Contributor Author

Done

@dbrgn dbrgn merged commit d5dbe30 into tealdeer-rs:main Jan 21, 2022
@cyqsimon cyqsimon deleted the no-auto-update branch January 21, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement waiting-for-author The PR needs an update before it can be considered for merging.
Development

Successfully merging this pull request may close these issues.

3 participants