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

feat: KTX module #516

Merged
merged 15 commits into from May 22, 2022
Merged

feat: KTX module #516

merged 15 commits into from May 22, 2022

Conversation

SubSide
Copy link
Contributor

@SubSide SubSide commented Jan 23, 2022

Prerequisites for Code Changes

  • This pull request follows the code style of the project
  • I have tested this feature

...

Additional Information

This will add a few kotlin convenience extension functions in a separate library so it can be thrown in if the person wants it.

image

@PhilippHeuer PhilippHeuer changed the base branch from master to develop January 23, 2022 21:21
Copy link
Member

@PhilippHeuer PhilippHeuer left a comment

Choose a reason for hiding this comment

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

Thanks a lot for creating this PR / working on a kotlin module :)

I left a few general comments, but should find the time to really review it in the next few days.

settings.gradle.kts Outdated Show resolved Hide resolved
@stachu540
Copy link
Contributor

I can at least add one more thing. For documentation please use Dokka. And generate javadoc using dokkaJavadoc task. About aggregate them... we do not have a clue to add them to website. Maybe in the future? Who knows?

Copy link
Member

@iProdigy iProdigy left a comment

Choose a reason for hiding this comment

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

Thanks for getting started on this!

build.gradle.kts Outdated Show resolved Hide resolved
Copy link
Contributor

@stachu540 stachu540 left a comment

Choose a reason for hiding this comment

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

I'd add some suggested changes, if @iProdigy / @PhilippHeuer have time to check. :)

twitch4j/build.gradle.kts Outdated Show resolved Hide resolved
twitch4j-ktx/build.gradle.kts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
compileOnly(project(":twitch4j"))

// Kotlin coroutines
api(group = "org.jetbrains.kotlinx", name = "kotlinx-coroutines-core")

This comment was marked as resolved.

Copy link
Contributor Author

@SubSide SubSide Jan 29, 2022

Choose a reason for hiding this comment

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

We're not using it, so there is no reason to include it.

Copy link
Contributor

Choose a reason for hiding this comment

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

so there is no reason to set kotlinOptions.jvmTarget in KotlinCompile tasks

Copy link
Member

Choose a reason for hiding this comment

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

don't need to explicitly add stdlib: https://kotlinlang.org/docs/gradle.html#dependency-on-the-standard-library

we could specify jvmTarget (but it already defaults to 1.8): https://kotlinlang.org/docs/gradle.html#attributes-specific-to-jvm

kotlin/build.gradle.kts Outdated Show resolved Hide resolved
Copy link
Contributor

@stachu540 stachu540 left a comment

Choose a reason for hiding this comment

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

Last thing todo is rename:
kotlin/src/main/java/com/github/twitch4j/kotlin/chat/ChatKtx.kt -> kotlin/src/main/java/com/github/twitch4j/kotlin/chat/Chat.kt
kotlin/src/main/java/com/github/twitch4j/kotlin/main/HystrixKtx.kt -> kotlin/src/main/java/com/github/twitch4j/kotlin/api/Hystrix.kt
kotlin/src/test/java/com/github/twitch4j/kotlin/chat/ChatKtxTest.kt -> kotlin/src/test/java/com/github/twitch4j/kotlin/chat/ChatTest.kt

@SubSide SubSide changed the title WIP: feat: KTX module feat: KTX module Feb 27, 2022
@SubSide SubSide marked this pull request as ready for review February 27, 2022 21:08
Copy link
Contributor

@stachu540 stachu540 left a comment

Choose a reason for hiding this comment

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

LGTM

@SubSide
Copy link
Contributor Author

SubSide commented May 22, 2022

👀

@PhilippHeuer PhilippHeuer merged commit 41905a4 into twitch4j:develop May 22, 2022
@iProdigy
Copy link
Member

Thanks for your contribution & apologies for the merge delay

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.

None yet

4 participants