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

Deglitcher interface for Views #647

Merged
merged 2 commits into from
Sep 6, 2024
Merged

Deglitcher interface for Views #647

merged 2 commits into from
Sep 6, 2024

Conversation

rpiaggio
Copy link
Collaborator

@rpiaggio rpiaggio commented Sep 1, 2024

No description provided.

@mergify mergify bot added the core label Sep 1, 2024
Copy link
Collaborator

@cquiroz cquiroz left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot added the tests label Sep 5, 2024
@rpiaggio
Copy link
Collaborator Author

rpiaggio commented Sep 5, 2024

Now this ready for review

@rpiaggio rpiaggio marked this pull request as ready for review September 5, 2024 23:19
Copy link
Contributor

@hugo-vrijswijk hugo-vrijswijk left a comment

Choose a reason for hiding this comment

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

Any chance for some tests? :)

@@ -3,7 +3,6 @@

package crystal

import cats.Eq
Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh. At first I read this as View Flaws Spec.

@rpiaggio
Copy link
Collaborator Author

rpiaggio commented Sep 6, 2024

Now this ready for review

yup, working on that

G.monotonic.flatMap(now => waitUntil.set(now + timeout))

private def throttlerView(view: ViewF[F, A]): ViewF[F, A] =
view.withOnMod(_ => curb) // todo try to curb before modding?? maybe not necessary
Copy link
Collaborator

Choose a reason for hiding this comment

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

The timeout is reset with each modification? Good.

Copy link
Collaborator

@toddburnside toddburnside left a comment

Choose a reason for hiding this comment

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

This looks great to me! But, I agree that some tests would be nice. :)

@rpiaggio
Copy link
Collaborator Author

rpiaggio commented Sep 6, 2024

Tests were added, and the fixes for the bugs they revealed.

@rpiaggio rpiaggio merged commit 797588a into master Sep 6, 2024
8 checks passed
@rpiaggio rpiaggio deleted the view-deglitcher branch September 6, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants