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

Better logging and separate reflect artifact #786

Merged
merged 124 commits into from
Apr 13, 2023
Merged

Conversation

alexander-yevsyukov
Copy link
Contributor

@alexander-yevsyukov alexander-yevsyukov commented Mar 15, 2023

This PR improves modularity of the base project, introducing two new modules, logging and reflect. The aim is to later extract these modules into a dedicated repositories and develop them separately.'

The logging module is organized as Kotlin multiplatform project. Several adjustments were necessary to adopt our build scripts for supporting Kotlin KMM. These adjustments will be migrated to config in one of the following PRs.

Logging API improvements

The extracted logging module is a Kotlin multi-platform module, which introduces fluent logging API inspired by Flogger but in Kotlin.

The logging API introduces a notion of LoggingDomain which allows to group classes with logging instructions into a namespace. This namespace is added as a prefix to log messages of these classes in the following format:

[$loggingDomain] $message

In addition to the LoggingDomain Kotlin annotation class, a Java annotation interface JvmLoggingDomain was introduced. The Java annotation allows to annotate both types and packages, and it is used for lookup of a logging domain declared in a package ”hierarchy” from innermost to outermost.

An expected object called LoggingFactory was introduced with the JVM implementation of it.

Kotlin classes in need of logging now may use WithLogging interface, which exposes a logger property. It is expected that the Logging interface currently used from both Java and Kotlin would be later deprecated in favor of WithLogging. But it's too early to say it for sure.

Other notable changes

  • The callsite of the LoggingApi provided by the Logging interface was fixed so that it refers to the real class which calls logging instructions.
  • The project structure was improved to push the adoption of script plugins even further.
  • Kotlin extensions for working with Dokka are now consolidated under buildSrc/.../kotlin/DokkaExts.kt, making the closer to the script plugins that use these extensions.
  • Configuration of Dokka for Kotlin and for Java now have dedicated script plugins.
  • The DSL for configuring JAR artifacts is extended to allow Dokka artifacts for Java and Kotlin. More work is needed to allow publication of Dokka artifact in Kotlin API mode.
  • Kotlin extensions of Gradle types for publishing are now consolidated under PublishingExts.kt.
  • The code of the base module no longer depends on reflection utilities. Only logging does.
  • A script plugin (jacoco-kmm-jvm.gradle.kts) was added to address specifics of code coverage in a KMM project.

Improvements to the reflect module

  • AnnotatedPackages class was introduced to simplify working with annotations of nested packages.

Deprecations and removals

  • Types.isMessageClass(Class) is deprecated in favor of Kotlin extension function Type.isMessageClass() introduced in the base module.
  • Properties of PublishingRepos for maven.teamdev.com were removed as these repositories are not used for quite a long time.

@alexander-yevsyukov alexander-yevsyukov self-assigned this Mar 15, 2023
.ruby-version Outdated Show resolved Hide resolved
}

@Test
fun `assume 'SEVER' for errors`() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fun `assume 'SEVER' for errors`() {
fun `assume 'SEVERE' for errors`() {

}

@Test
fun `obtain same 'FluentLogger' Instance`() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fun `obtain same 'FluentLogger' Instance`() {
fun `obtain same 'FluentLogger' instance`() {

Comment on lines 34 to 40
module
`maven-publish`
`kotlin-jvm-module`
`dokka-for-kotlin`
idea
`project-report`
`detekt-code-analysis`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can kotlin-jvm-module apply module on its own? It looks like kotlin-jvm-module is already module but with some additional stuff.

And some of these plugins are already applied in module. This block can be shortened.

Comment on lines 117 to 118
var handle = Invokables.asHandle(method);
Truth.assertThat(handle).isNotNull();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like imports were badly optimized. Static imports are in place, but seem to be not used.

Copy link
Collaborator

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@alexander-yevsyukov LGTM except for the comment related to Ruby set up.

Copy link
Collaborator

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@alexander-yevsyukov please see a couple more minor comments.

rootFolder.set(rootDir)
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is redundant.

@@ -117,38 +113,38 @@ void throwOnInvocationError() throws NoSuchMethodException {
@DisplayName("convert a visible method to a handle")
void convertToHandle() throws Throwable {
var method = ClassWithPrivateMethod.class.getMethod("publicMethod");
var handle = asHandle(method);
var handle = Invokables.asHandle(method);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe, we should rollback to static imports, as previously?

@@ -62,50 +56,41 @@ class TypesTest extends UtilityClassTest<Types> {
@Test
@DisplayName("create a map type")
void createMapType() {
var type = mapTypeOf(String.class, Integer.class);
var type = Types.mapTypeOf(String.class, Integer.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe, we should use static imports, as before?

Copy link
Collaborator

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@alexander-yevsyukov alexander-yevsyukov added this pull request to the merge queue Apr 13, 2023
Merged via the queue into master with commit 9782594 Apr 13, 2023
@alexander-yevsyukov alexander-yevsyukov deleted the better-logging branch April 14, 2023 16:53
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