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 --auto-update flag to allow automatically updating the cache #73

Closed
wants to merge 5 commits into from
Closed

Add --auto-update flag to allow automatically updating the cache #73

wants to merge 5 commits into from

Conversation

rnestler
Copy link

@rnestler rnestler commented Jan 7, 2019

When not updating the cache manually one often gets the following
message:

Cache wasn't updated in 30 days.
You should probably run `tldr --update` soon.

The auto-update flag allows to just alias tldr to tldr -a which will
automatically update the cache if it is older than the maximum age.

… out of date

When not updating the cache manually one often gets the following
message:

    Cache wasn't updated in 30 days.
    You should probably run `tldr --update` soon.

The auto-update flag allows to just alias tldr to tldr -a which will
automatically update the cache if it is older than the maximum age.
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.

Nice, thanks!

src/main.rs Outdated Show resolved Hide resolved
};
});
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good to print an information if the cache was successfully updated:

if !args.flag_quiet {
    println!("Successfully updated cache.");
}

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but since I don't abort with the '-a' flag when updating fails I'll need to do it with a .map call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, or match :)

Copy link
Author

@rnestler rnestler Jan 7, 2019

Choose a reason for hiding this comment

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

I did it with map, didn't see your comment before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest match would be much more readable in this case. Don't you think?

Copy link
Author

Choose a reason for hiding this comment

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

I love the bike-shedding here 😆 I don't like the double OK(_) pattern with the somewhat unrelated if guard, but I don't care that much. Just decide for the style you prefer most 😃

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, you argued with length, and this version is shorter. And I love my bikeshed in darker hues of blue! Light blue is just abhorrent...

Copy link
Collaborator

@dbrgn dbrgn Jan 7, 2019

Choose a reason for hiding this comment

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

And if you prefer it, you can also use this:

match cache.update() {
    Ok(_) => if !args.flag_quiet => {
        println!("Successfully updated cache.");
    },
    Err(e) => match e {
        CacheError(msg) | ConfigError(msg) | UpdateError(msg) => {
            eprintln!("Could not update cache: {}", msg)
        }
    },
}

I think that's actually the nicest shade of medium blue 🙂

Copy link
Author

Choose a reason for hiding this comment

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

@dbrgn Ping. I applied your favorite shade of medium blue 😉

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I've been sick for the last three days. Will take a look in a few days.

@dbrgn
Copy link
Collaborator

dbrgn commented Jan 12, 2019

Thanks for the update.

I thought about this PR a bit. I'm still not sure if the command line flag is the right place for such a setting. It's not something you would want for some command invocations but not for others, so the config file would probably make more sense, right?

This would require a slight reorganization of the main function, but much less than I initially thought:

  • Handle args.flag_config_path and args.flag_seed_config right after handling args.flag_version (simply move up the code blocks)
  • Move the config loading code in print_page into the main function, right after the args.flag_seed_config handling block
  • In src/config.rs, add the new config elements to the RawConfig and Config structs and add the defaults to RawConfig::new().

The default configuration can be generated with tldr --seed-config. My suggestion would be:

[updates]
auto_update = false
auto_update_interval_hours = 720

(The auto_update_interval_hours could then be set to MAX_CACHE_AGE * 24 by default.)

What do you think? Sorry for the change request, but every new flag is a maintenance burden because removing it later is a breaking change, so I want to think twice before adding such new flags...

@rnestler
Copy link
Author

What do you think? Sorry for the change request, but every new flag is a maintenance burden because removing it later is a breaking change, so I want to think twice before adding such new flags...

The same is true for config options 😉 But probably we want to think about the general configuration concept. Some tools have almost every option configurable via command line, config file and environment variables. But this may or may not make sense for tealdeer.

But for the auto update the config file probably makes most sense since it is a rather static option.

@dbrgn
Copy link
Collaborator

dbrgn commented Mar 10, 2019

But for the auto update the config file probably makes most sense since it is a rather static option.

👍

@devnoname120
Copy link

Any updates on this?

@dbrgn
Copy link
Collaborator

dbrgn commented Oct 22, 2019

Any updates on this?

The changes outlined in #73 (comment) would need to be implemented.

@rnestler are you still interested in implementing those changes, or should we close the PR for now so somebody else can take over?

@rnestler
Copy link
Author

@rnestler are you still interested in implementing those changes, or should we close the PR for now so somebody else can take over?

This PR is probably the wrong place to implement this anyways. So let's close it and me or somebody else may open a new one.

@rnestler rnestler closed this Oct 22, 2019
@dbrgn dbrgn mentioned this pull request Oct 22, 2019
@dbrgn
Copy link
Collaborator

dbrgn commented Oct 22, 2019

Opened an issue at #90.

@rnestler rnestler deleted the auto-update branch October 24, 2019 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants