lichess-org / scalachess

Chess API written in scala. Immutable and free of side effects.
https://lichess.org
MIT License
676 stars 208 forks source link

Simplify removeById #550

Closed Masynchin closed 6 months ago

Masynchin commented 6 months ago

I failed to run tests and scared to run benchmarks, so I will rely on CI. I hope this code works the same as previous one, and improves performance.

ornicar commented 6 months ago

does this not reverse the list? I suppose a new test would be nice here

Masynchin commented 6 months ago

does this not reverse the list? I suppose a new test would be nice here

Nope, I just reimplemented filter which propagates predicate only once. Here is the quick scastie demo:

Scastie proof that is doesn't reverse the list
lenguyenthanh commented 6 months ago

does this not reverse the list? I suppose a new test would be nice here

We already have tests, but not for preserving order. I did add those tests.

Also the name of this function is misleading, it actually only removes the first item that have the correspondent id. Added a test and comments to clarify it as well.

lenguyenthanh commented 6 months ago

I hope this code works the same as previous one, and improves performance.

I believe I wrote the code that way to avoid recursion, but your solution can be recursiveless as well with tailrec annotation. So, the performance should be at least the same (maybe an actual benchmark will tell different story). And this is indeed looks better than before.

I failed to run tests and scared to run benchmarks, so I will rely on CI.

What is your problem? I'd like to help you with your local setup if you want. And thanks for pr!

Masynchin commented 5 months ago

What is your problem? I'd like to help you with your local setup if you want. And thanks for pr!

@lenguyenthanh sorry for the delay, but I would be thankful if you help me. I was trying to run sbt testKit / test, but it failed:

Traceback ~~~plain [info] welcome to sbt 1.10.0 (GraalVM Community Java 17.0.8) [info] loading global plugins from HOME/.sbt/1.0/plugins [info] loading settings for project scalachess-build from plugins.sbt ... [info] loading project definition from HOME/code/scalachess/project [info] loading settings for project root from build.sbt ... [info] set current project to root (in build file: HOME/code/scalachess/) [error] Not a valid command: testKit (similar: exit) [error] Expected '/' (if selecting a project) [error] Expected ':' [error] Not a valid key: testKit (similar: test, testFilter, testQuick) [error] testKit [error] ^ ~~~

Then I have tried sbt testKit/test, but it also failed:

Traceback ~~~plain [info] welcome to sbt 1.10.0 (GraalVM Community Java 17.0.8) [info] loading global plugins from HOME/.sbt/1.0/plugins [info] loading settings for project scalachess-build from plugins.sbt ... [info] loading project definition from HOME/code/scalachess/project [info] loading settings for project root from build.sbt ... [info] set current project to root (in build file: HOME/code/scalachess/) [info] compiling 80 Scala sources to HOME/code/scalachess/core/target/scala-3.4.2/classes ... [error] 21 is not a valid choice for -java-output-version [info] scalac -help gives more information [error] one error found [error] (scalachess / Compile / compileIncremental) Compilation failed [error] Total time: 4 s, completed 12 июн. 2024 г., 07:21:44 ~~~
lenguyenthanh commented 5 months ago

[error] 21 is not a valid choice for -java-output-version

This is the problem, You need to use jdk 21 to compile/run/test

Masynchin commented 5 months ago

This is the problem, You need to use jdk 21 to compile/run/test

Thanks for the help! I'll upgrade it