Skip to content

Conversation

@seroperson
Copy link
Contributor

Continuing #383 (comment) and #485 discussion:

What I did here:

  • e1325b4:
    • -rewrite -source:3.7-migration.
    • Some explicit Decoder definitions, as they weren't resolved otherwise.
    • Moved src-jvm to src-jvm2, because it also depends on scalaj-http.
  • Trying to workaround a bug I mentioned here (Scala 3 #383 (comment)), 3045a65:
    • Splitting CirceDecoders, moving Decoder definitions to their models
  • After I splitted CirceDecoders, now the same StackOverflowException were occuring on CirceEncoders 🤡, so I splitted it the same way too, bf55b75
  • Then Scala3 finally compiled, but Scala2 build broke for some reason with unresolved implicits in RequestHandler.scala. I guess that's because of different implicit resolution mechanism on Scala2 and Scala3, and Scala2 isn't clever enough to actually match R with Request's type parameter and find implicit in its' object. That's why I've rewritten it using dependent types, b9bd2d2 (see Request, RequestHandler and implementations):
  • With dependent types instead of type parameter approach it now looks cleaner and everything compiles both for Scala2 and Scala3. I had to broke compatibility a little bit, but I hope it's excusable.

I haven't tested it in real application yet, don't have anything suitable at the moment, but at least tests are green.

Thank you for your attempt @viperey and @ex0ns for your great library 👍

@ex0ns
Copy link
Contributor

ex0ns commented Jul 6, 2025

Thanks a lot @seroperson
I don't really mind the breaking changes if that means that we can move this library to scala 3. And the dependent type instead of it being a type parameter looks very equivalent to me (even though I don't understand why this was working before and it does not anymore, maybe you have a guess about this ?)

I will try to upgrade some small project of mine with this version (without migrating them to scala 3), see how painful/not painful the process is. Hopefully I should be able to do that today.

Let me know when you have the chance to test it on a real app, that might also be of interest for @viperey as they had the need to a version that support scala 3.

@ex0ns
Copy link
Contributor

ex0ns commented Jul 6, 2025

@seroperson the CI is failing because of mill version, I will upgrade that right now

Edit: now the CI has the same stack overflow error you had before I believe. I don't have them locally

@seroperson
Copy link
Contributor Author

seroperson commented Jul 6, 2025

@seroperson the CI is failing because of mill version, I will upgrade that right now

Edit: now the CI has the same stack overflow error you had before I believe. I don't have them locally

Well, that's weird, I don't have it locally too. Before CirceDecoders + CirceEncoders splitting, it occurred locally too, I guess this bug somehow depends on computing resources and amount of auto-generated things. I see now it fails on Message generating, which is heavy enough. I can try to implement the decoder manually.

And the dependent type instead of it being a type parameter looks very equivalent to me (even though I don't understand why this was working before and it does not anymore, maybe you have a guess about this ?)

I guess that's because previously all decoders were in scope explicitly because they were defined right in the single CirceDecoders and were explicitly imported. After I moved them to corresponding models, I guess it wasn't smart enough to find them in companion object of R. Actually it sounds fair.

@ex0ns
Copy link
Contributor

ex0ns commented Jul 6, 2025

Great ! CI is now green. I still need to find the time to upgrade my projects to test now

@seroperson
Copy link
Contributor Author

Great ! CI is now green. I still need to find the time to upgrade my projects to test now

ofc, it's better to wait than release bad package. This week I'll try too I think.

@ex0ns
Copy link
Contributor

ex0ns commented Jul 6, 2025

I added a comment based on a test I just did.

The good news is that I upgraded one of my simple project and I had to change exactly 0 lines (public API seems to be untouched), I believe that I will still release it as 6.0.0 when we are sure that it works.

Changing Future.unit will null made the example (RandomBot) work

@seroperson
Copy link
Contributor Author

seroperson commented Jul 7, 2025

I've migrated my bots to Scala3 and they seem run okay too 🎉At least locally.

private val apiBaseUrl = s"https://$telegramHost/bot$token/"

override def sendRequest[R, T <: BotRequest[_]](request: T)(implicit encT: Encoder[T], decR: Decoder[R]): F[R] = {
override def sendRequest[T <: BotRequest: Encoder](
Copy link

Choose a reason for hiding this comment

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

@ex0ns out of curiosity, did your test hit this area of the code?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would have to double check as I only did a very small set of tests, but I would say yes as RandomBot is based on ExampleBot which should be using the SttpClient.

Let me make sure of it 👌

Copy link

Choose a reason for hiding this comment

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

Okay, that line change broke my private code, but, minor adjustments apart, it works.
In general the new version seems to be smoothly working, great job.

cc/ @seroperson

Copy link
Contributor Author

@seroperson seroperson Jul 8, 2025

Choose a reason for hiding this comment

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

yea, it broke mine too just a little. Well, anyway I guess we're going to up major version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be able to share what you changed? I will include it in the release notes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this?:

- Circe Codecs from `com.bot4s.telegram.marshalling.CirceDecoders` and `com.bot4s.telegram.marshalling.CirceEncoders` were moved to corresponding models.
- `com.bot4s.telegram.methods.Request`, `com.bot4s.telegram.methods.JsonRequest` and `com.bot4s.telegram.methods.MultipartRequest` aren't type-parameterized anymore, `R` type parameter was replaced with a dependent type `Response`.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was more talking about what you had to change in the code of your bots to align with the new version, but I guess it was mostly removing type parameters ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yea, well, just aligned sendRequest signatures with new changes. It'll only affect you in case of custom RequestHandler or if you're using Request type somewhere.

For example, I have a custom ZIORequestHandler, which I had to change like this:
image

@ex0ns
Copy link
Contributor

ex0ns commented Jul 10, 2025

Thanks a lot for the work ! I'm going to merge that. Due to some changes to the publication of the artifact, we will not have a snapshot release for this one yet, but I will make sure to publish version 6.0.0 before the end of the week

It will be hard to test/cover each an every scenario that this lib cover in the current state, but I updated a few of my bot, and ran the example available in this repo and they all worked as expected

@ex0ns
Copy link
Contributor

ex0ns commented Jul 14, 2025

I upgraded to mill 1.0 in the meantime to have working snapshot, I will now merge this on main and leave the snapshot to be tested for a few days before doing the proper release.

@ex0ns ex0ns merged commit 65f7d08 into bot4s:main Jul 14, 2025
1 check passed
@ex0ns
Copy link
Contributor

ex0ns commented Jul 14, 2025

5.8.4-65f7d08f-SNAPSHOT is now available

@seroperson
Copy link
Contributor Author

5.8.4-65f7d08f-SNAPSHOT is now available

Is it really available? Where can I find it?

[error]   not found: https://repo1.maven.org/maven2/com/bot4s/telegram-core_3/5.8.4-65f7d08f-SNAPSHOT/telegram-core_3-5.8.4-65f7d08f-SNAPSHOT.pom
[error]   not found: https://central.sonatype.com/repository/maven-snapshots/com/bot4s/telegram-core_3/5.8.4-65f7d08f-SNAPSHOT/telegram-core_3-5.8.4-65f7d08f-SNAPSHOT.pom
[error]   not found: https://oss.sonatype.org/content/repositories/snapshots/com/bot4s/telegram-core_3/5.8.4-65f7d08f-SNAPSHOT/telegram-core_3-5.8.4-65f7d08f-SNAPSHOT.pom

@ex0ns
Copy link
Contributor

ex0ns commented Jul 25, 2025

I'm sorry it's not, sonatype changed recently and it failed silently.
I'm getting The version cannot be a SNAPSHOT on maven publish, which I don't understand and it's not very documented... will investigate

https://mill-build.org/mill/scalalib/publishing.html#_snapshot_versions

@ex0ns
Copy link
Contributor

ex0ns commented Aug 9, 2025

For future reference:
image
image

This might be coming from mill

image (in the 1.0.1 release). I will upgrade mill

@ex0ns
Copy link
Contributor

ex0ns commented Aug 9, 2025

@seroperson the CI ran:

 [526] Detected a 'SNAPSHOT' version for Artifact(com.bot4s,telegram-core_3,6.0.0-SNAPSHOT), publishing to Sonatype Central Snapshots at 'https://central.sonatype.com/repository/maven-snapshots/'
[526] Publishing to 'https://central.sonatype.com/repository/maven-snapshots/' finished with result: DeployResult(
[526]   artifacts = [
[526]     com.bot4s:telegram-core_3:pom:6.0.0-20250809.145419-1
[526]     com.bot4s:telegram-core_3:jar:6.0.0-20250809.145419-1
[526]     com.bot4s:telegram-core_3:jar:sources:6.0.0-20250809.145419-1
[526]     com.bot4s:telegram-core_3:jar:javadoc:6.0.0-20250809.145419-1
[526]   ]
[526]   metadatas = [
[526]     com.bot4s:telegram-core_3:6.0.0-SNAPSHOT/maven-metadata.xml
[526]     com.bot4s:telegram-core_3/maven-metadata.xml
[526]   ]
[526] )

@seroperson
Copy link
Contributor Author

@seroperson the CI ran:

Thank you!

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