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

Fix: documentation #143

Merged
merged 7 commits into from
Sep 17, 2023
Merged

Fix: documentation #143

merged 7 commits into from
Sep 17, 2023

Conversation

RichDom2185
Copy link
Contributor

@RichDom2185 RichDom2185 commented Sep 17, 2023

Based on the changes to the example in #138, it seems that the intended use case is still to specify the interval in minutes:

But in the refactoring done in #130, the interval option is passed directly into the worker, without going through the minute-to-millisecond conversion.

This PR restores the original behavior, and also updates the outdated example in the README following the refactor.

If we don't want to keep this minute-to-ms behavior, the alternative is to update the examples accordingly; in that case, this PR could be the starting point for the discussion.

Fix outdated documentation following breaking change refactor.
* Restore `minute_to_ms` function
* Pipe interval option through `minute_to_ms`
@RichDom2185 RichDom2185 requested a review from a team as a code owner September 17, 2023 15:38
@RichDom2185
Copy link
Contributor Author

Regarding this, I also feel that the v3.0.0 Breaking Changes esction in CHANGELOG.md should be updated to include the fact that the sweep_interval option now does nothing, following #130.

In the same vein, these lines should be removed from lib/guardian/db.ex.

Let me know what you think of these and I'll make the changes accordingly.

@yordis
Copy link
Member

yordis commented Sep 17, 2023

hey there, ideally, we take ms and avoid any conversion

@RichDom2185
Copy link
Contributor Author

hey there, ideally, we take ms and avoid any conversion

Ok, in that case, shall we update the documentation instead? I've updated the PR accordingly.

@RichDom2185 RichDom2185 changed the title Fix: configure interval by minutes Fix: documentation Sep 17, 2023
@yordis yordis merged commit aed1cc4 into ueberauth:master Sep 17, 2023
2 checks passed
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