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

Update Tickler to use threading events for stopping. #67

Merged
merged 3 commits into from
Mar 5, 2025

Conversation

lach1010
Copy link
Contributor

@lach1010 lach1010 commented Mar 3, 2025

Noticed that shutdown is very slow when using oAuth due to secondary thread in Tickler waiting for sleep(60) to end.

This update uses threading events to wake the thread immediately on .stop()

Wasn't able to run all the tests due to the test_utils.py usage of _testcapi which isn't available in Python as packaged by uv.

Couple project settings changes to support dev on MacOS and developers with Ruff installed.

Would be curious to hear thoughts on using Ruff in this project as formatter and linter.

PS. This is my first contribution to a third party package. What's the best way test changes outside of unit tests?

@Voyz
Copy link
Owner

Voyz commented Mar 4, 2025

Hey @lach1010 thanks for your PR! The replacement of self._running for threading.Event is great, I appreciate you spotting it and contributing a solution 👍

You seem to intertwine three other subjects into this PR: the tests not working on your MacOS, support for Ruff and contributing guidelines. In general it would be best for each to be addressed in a separate issue, rather than bundled up.

As for tests not working on your MacOS, please open a separate issue to discuss it.

As for Ruff, there's an open PR adding it in: #50

As for contributing guidelines, I'm working on these. In the meantime, please open a separate issue to discuss the question you've posted.


As for this PR, when you find a minute please remove the pyproject.toml / Ruff additions, and I'll be happy to merge it in. Thanks!

@lach1010
Copy link
Contributor Author

lach1010 commented Mar 4, 2025

Thanks mate.

I've removed the other changes. Will open another issue for them.

@Voyz
Copy link
Owner

Voyz commented Mar 5, 2025

Great job! Merging this in now, thanks for the contribution! 🙌

@Voyz Voyz merged commit 6f6767c into Voyz:master Mar 5, 2025
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