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

New Spoon - Hydrate - Drink Water reminder #257

Closed
wants to merge 2 commits into from

Conversation

luizanao
Copy link

@luizanao luizanao commented Jan 2, 2022

Hydrate

This is a very simple spoon that emits a system pop-up notification every X hours to remind folks to drink water.

Usage:

hs.loadSpoon("Hydrate")
spoon.Hydrate.reminder_interval_minutes = 60
spoon.Hydrate.reminder_days = {"Monday", "Wednesday", "Friday"}
spoon.Hydrate:start()

Source/Hydrate.spoon/init.lua Show resolved Hide resolved
"Sunday"
}

--- Hydrate.Notifier
Copy link
Member

Choose a reason for hiding this comment

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

This API is going to be tricky to express usefully with the way our docstrings work, because Notifier isn't defined anywhere as a module, and in fact it's not a separate module (and if it were, it would get its own page in the docs).

I wonder if it would be possible to restructure things a bit so the API is all in the Hydrate namespace?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the advice @cmsj . This is literally my first interaction w/ hammerspoon (and lua). I will take a look in other projects and the docs and come up w/ some solution :)

Copy link
Member

Choose a reason for hiding this comment

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

In theory, if you rebase your branch off our master branch, you should get docstrings errors as comments now. I'm pretty sure I've finally figured out a way to make that work :)

Copy link
Author

Choose a reason for hiding this comment

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

just rebased, lets see :)

Copy link
Author

Choose a reason for hiding this comment

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

I rebased asap I saw your comment @cmsj but I guess the changes weren't in master yet, due to CI delays. I just did it again now. Could you allow CI to run again pls?

@cmsj cmsj marked this pull request as draft January 14, 2022 11:47
@luizanao luizanao marked this pull request as ready for review January 14, 2022 13:47
@luizanao luizanao requested a review from cmsj January 14, 2022 13:47
just a reminder to help you drink water regurlarly
@cmsj
Copy link
Member

cmsj commented Aug 7, 2024

@luizanao I know it's been a while since we last discussed this PR, but do you want to proceed with merging it? If so we'll need to figure out a way to handle the docstring issue.

@luizanao
Copy link
Author

luizanao commented Aug 7, 2024

I am good just closing this out @cmsj - life has been busy and I wont have the time to work on that soon. Dont worry, I am hydrating as much as I can tho!

@luizanao luizanao closed this Aug 7, 2024
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.

2 participants