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

[RKOTLIN-1038] Implement new log category for RealmLogger #1692

Merged
merged 60 commits into from
May 17, 2024

Conversation

clementetb
Copy link
Contributor

closes #1691

RealmLog would see the current level property deprecated in favor of a setter and getter functions.

RealmLog {
    @Deprecated
    public var level: LogLevel

    public fun setLevel(level: LogLevel, category: LogCategory)
    public fun getLevel(category: LogCategory): LogLevel
}

The custom logger interface also gets modified adding a new method that has the category in its signature. Backward compatibility would be ensured to the current deprecated methods.

RealmLogger {
    public fun log(
        category: LogCategory, 
        level: LogLevel, 
        throwable: Throwable?, 
        message: String?, 
        vararg args: Any?
    )
}

@clementetb clementetb self-assigned this Mar 15, 2024
@cla-bot cla-bot bot added the cla: yes label Mar 15, 2024
@clementetb clementetb requested a review from rorbech March 15, 2024 12:18
@clementetb clementetb changed the base branch from main to cr/core-upgrade March 15, 2024 13:18
@@ -20,3 +21,31 @@ public enum class LogLevel(public val priority: Int) {
WTF(6),
NONE(7);
}

internal fun LogLevel.toCoreLogLevel(): CoreLogLevel {
Copy link
Contributor

Choose a reason for hiding this comment

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

Internal stuff should go into an internal package/class/file

Copy link
Contributor Author

@clementetb clementetb May 9, 2024

Choose a reason for hiding this comment

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

These internal helper functions will not be visible to the end-user, unless they inspect the code.

Wasn't the original purpose of the internal package to hide internal classes that had to be made public because of the base/sync split?

I think having these two helper functions next to the enum they are helping makes for better discoverability, rather than having to jump between files to see if all enum elements are listed or not.

I don't have a strong opinion, if you like them moved I will displace them 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for the internal packages was to split it so that it was obvious what the public API was and that users wouldn't end up browsing internal code unless they intentionally navigated to it from the public API. We haven't been completely strict on this, but I don't see why we should not try to follow it 🤷

package io.realm.kotlin.log

/*
* Core does not expose the different categories in compile time.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move all the internal stuff to an internal package/class/file. It confuses users if they end up here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this comment belong here. Seems like it is dangling and that the code has moved.

Copy link
Contributor

@rorbech rorbech left a comment

Choose a reason for hiding this comment

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

Nice clean ups. I like the idea of always logging to a specific logger instead of RealmLog.

There are some minors and then we need to bump the version and highlight the breaking changes.

@@ -9,6 +9,7 @@ This release will bump the Realm file format from version 23 to 24. Opening a fi
### Enhancements
* Support for RealmLists and RealmDictionaries in `RealmAny`. (Issue [#1434](https://github.com/realm/realm-kotlin/issues/1434))
* Optimized `RealmList.indexOf()` and `RealmList.contains()` using Core implementation of operations instead of iterating elements and comparing them in Kotlin. (Issue [#1625](https://github.com/realm/realm-kotlin/pull/1666) [RKOTLIN-995](https://jira.mongodb.org/browse/RKOTLIN-995)).
* Add support for filtering logs by category. (Issue [#1691](https://github.com/realm/realm-kotlin/issues/1691) [JIRA](https://jira.mongodb.org/browse/RKOTLIN-1038))
Copy link
Contributor

Choose a reason for hiding this comment

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

With the new interface this is actually a breaking change, and we should bump the version to 2.0.0-SNAPSHOT.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also highlight that the LogConfiguration is gone.

package io.realm.kotlin.log

/*
* Core does not expose the different categories in compile time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this comment belong here. Seems like it is dangling and that the code has moved.

@rorbech rorbech merged commit fd5a5ca into main May 17, 2024
56 checks passed
@rorbech rorbech deleted the ct/loging-categories branch May 17, 2024 07:10
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement new log category for RealmLogger
2 participants