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

Simplify Executor.acquire #267

Merged
merged 5 commits into from
Feb 18, 2024
Merged

Simplify Executor.acquire #267

merged 5 commits into from
Feb 18, 2024

Conversation

Masynchin
Copy link
Contributor

Same aim as in #266.

First, I extracted filtering logic from pattern match by applying filter method before, which also simplified pattern match to 1 case arm. Next I replaced foldLeft[Option] by reduceLeftOption, a little cleaner, but logic exact the same. And finally, I noticed that after filtering works with _.nonAcquired, the m.canAcquire(key) part of if-statement inside always evaluates to true because of the Work.Move implementation:

https://github.com/Masynchin/lila-fishnet/blob/2135cb36973495e55bc8909710063e7f388959e5/app/src/main/scala/Work.scala#L34-L36

So, reduce's if-statement shortens to m.createdAt.isBefore(a.createdAt), which is just comparing who created earliest. Only thing I don't know what is the logic when two Move's createdAt equals, or can it be at all

@lenguyenthanh
Copy link
Member

Another wonderful PR, I can't not express how much I love this kind PR! Really appreciate!

I read each commit carefully, I think they're correct and beautiful. It is just I'll be more confident if we have some tests for this logic.

Is this something you would like to try? If you don't I'll try to write some tests before merging this. Don't hesitate if you have any question.

@ornicar ornicar merged commit 76f63dd into lichess-org:master Feb 18, 2024
2 checks passed
@Masynchin
Copy link
Contributor Author

It is just I'll be more confident if we have some tests for this logic. Is this something you would like to try?

I was trying to write some test, but stuck up with IO.realTimeInstant mocking. My only solution was to provide custom Clock for IO, but it seems to be not working :(

https://scastie.scala-lang.org/rWkmMfYsTUGmRAjYg8hksQ

@lenguyenthanh
Copy link
Member

We can start with testing the logic inside ref.modify. Which is if we have a Map[WorkId, Work.Move] and a time at then we return the earliest non-acquired work. So, we don't have to deal with IO or mocking.

@Masynchin
Copy link
Contributor Author

We can start with testing the logic inside ref.modify.

Oh, now I can see that we can provide moves directly through ref, not only through making request. Thanks, I will give it another try

@lenguyenthanh
Copy link
Member

Only thing I don't know what is the logic when two Move's createdAt equals, or can it be at all

Forgot to mention this, We don't really need to care about that case. All we need is eventually all works will be acquirred.

@Masynchin
Copy link
Contributor Author

@lenguyenthanh sorry, I am too busy with studying and contributing for Cats (if Cats-PR got merged, I will push another PR here, sometime), would not be able to write tests any time soon ☹️

@lenguyenthanh
Copy link
Member

no worries @Masynchin, good luck on your cats's pr. Would always welcome your PRs!

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.

3 participants