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

LogHandler removal & logging improvements #340

Merged
merged 11 commits into from
Mar 4, 2022
Merged

Conversation

ikurek
Copy link
Contributor

@ikurek ikurek commented Feb 25, 2022

This should resolve #238, although it does not use the mentioned LogHandler. Instead, the changes contain:

  • Removing the LogHandler entirely from Dart code
  • Replacing Android native logging with Log implementation from ably-java. Luckily, their implementation is the same so it was only a matter of imports
  • Replacing ios print and NSLog calls with ARTLog implementation from ably-cocoa. This was not possible everywhere, because unlike Log from ably-java the ARTLog isn't a singleton, so I only updated a few places

With these changes, most of the logging from Android and iOS platform native code should respect logging level set in Ably ClientOptions.

@ikurek ikurek self-assigned this Feb 25, 2022
@github-actions github-actions bot temporarily deployed to staging/pull/340/dartdoc February 25, 2022 17:30 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/340/dartdoc February 25, 2022 17:35 Inactive
Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

Just a thought. What do you / others think?

@ikurek ikurek force-pushed the fix/log-handler-removal branch from 9ca4ad8 to 19de88c Compare March 2, 2022 14:36
@github-actions github-actions bot temporarily deployed to staging/pull/340/dartdoc March 2, 2022 14:39 Inactive
Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good, just a couple of tweaks before you merge this, please...

lib/src/authentication/src/client_options.dart Outdated Show resolved Hide resolved
lib/src/logging/src/log_handler.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@ikbalkaya ikbalkaya left a comment

Choose a reason for hiding this comment

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

Quickly scanned through changes, all looks good to me

@ikurek ikurek merged commit 1dfc9f7 into main Mar 4, 2022
@ikurek ikurek deleted the fix/log-handler-removal branch March 4, 2022 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix LogHandler (use it)
4 participants