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 functionality #270

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rcloran
Copy link
Contributor

@rcloran rcloran commented Oct 10, 2023

Fixes #102

Before submitting your PR (pull request), please
check the following:

  • There is no other PR (open or closed)
    similar to yours. If there is, please first
    discuss over there.
  • Each commit messages follows the Seven
    Rules of a Great Commit
    Message
    .
  • Each commit message includes
    Fixes #<bug> or Resolves #<issue> in its
    body (not subject) for each issue it resolves
    (if any).
  • You have
    squashed
    any redundant or insignificant commits.

Copy link
Contributor Author

@rcloran rcloran left a comment

Choose a reason for hiding this comment

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

There's quite a few places where I might've missed sticking to the same style/naming conventions as other places in znap. I'm happy to change this based on suggestions!

@@ -48,6 +48,11 @@ To run `znap pull` on specific repos only, including ones you have set to be exc
% znap pull <repo> ...
```

To be prompted to automatically update with `znap pull` every 30 days, add the following to your `.zshrc` file:
```sh
zstyle ':znap:pull' auto-update 30
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm more than happy to take feedback on naming of the zstyle -- not sure what the most consistent with the other styles would be. This is different in that it doesn't have a repo name as part of it. Also not sure if this should include "interval" or "days" or something as part of the name.

Also, it could be possible to have styles on whether or not to prompt.

@@ -58,6 +58,10 @@ zstyle -T :znap: auto-compile &&
add-zsh-hook preexec ..znap.auto-compile
add-zsh-hook zsh_directory_name ..znap.dirname

if zstyle -s ':znap:pull' auto-update _ ; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure whether it's better to test here or not. We have to test in the function, or pass the interval as a parameter (which just means setting a local here in init). My thinking was that we can avoid reading the file if the style is not set 🤷

scripts/init.zsh Outdated Show resolved Hide resolved
@rcloran
Copy link
Contributor Author

rcloran commented Oct 10, 2023

It's also not clear from the issue whether a prompt like this is what's wanted, or rather integration with the myriad cron-like systems out there, which would be quite a lot of work.

If what I've written isn't desired in znap, I won't be upset at all, happy to abandon it. I'd written it anyways for myself, since this is the functionality that I wanted on my machines.

@rcloran rcloran force-pushed the auto-update branch 2 times, most recently from 4dc13ad to 8902235 Compare October 10, 2023 20:20
@rimmer
Copy link

rimmer commented Oct 10, 2023

Looking forward to have it merged

@marlonrichert
Copy link
Owner

It's also not clear from the issue whether a prompt like this is what's wanted, or rather integration with the myriad cron-like systems out there, which would be quite a lot of work.

I think it's a bad idea to check on each shell startup whether to update. I would rather solve it in way that's similar to how git maintenance does it. See https://git-scm.com/docs/git-maintenance#Documentation/git-maintenance.txt---schedulerautocrontabsystemd-timerlaunchctlschtasks and the sections below it.

Are you interesting in updating your pull request to provide the initial framework for this? You don't need to implement support for all possible cron-like systems immediately. Just start with one, like crontab or launchctl, and we can add the rest later. Just keep it simple.

Thanks for contributing! 🙂

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.

Add optional auto-update feature
3 participants