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

Add global SDK logger #242

Merged
merged 21 commits into from
May 31, 2024
Merged

Add global SDK logger #242

merged 21 commits into from
May 31, 2024

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented May 28, 2024

This PR adds a global logger similar (almost identical) to Breez SDK.

To setup logging, the Rust interface uses LiquidSdk::init_logging(data_dir, logger) and the uniffi bindings use set_log_stream(LogStream log_stream).

Closes #236

Copy link
Contributor

@erdemyerebasmaz erdemyerebasmaz left a comment

Choose a reason for hiding this comment

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

I suggest having a look at libs/sdk-core/src/logger.rs on:

To ensure env_logger module filters are applied on Dart & UniFFI loggers.

I can also apply these changes in scope of another PR.

@ok300
Copy link
Contributor Author

ok300 commented May 29, 2024

@erdemyerebasmaz looks like that's still under review, so it's not in its final form and might still undergo changes.

How about replicating that change here once that PR is merged, and until then sticking with this PR?

lib/bindings/src/lib.rs Outdated Show resolved Hide resolved
packages/react-native/src/index.ts Outdated Show resolved Hide resolved
# Conflicts:
#	lib/core/src/sdk.rs
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Looks good, small NITs.

lib/bindings/src/breez_liquid_sdk.udl Outdated Show resolved Hide resolved
lib/core/src/sdk.rs Outdated Show resolved Hide resolved
@dangeross
Copy link
Collaborator

@ok300 I saw you added the set_log_stream to the ignore list in the RN codegen, but you also need a add the manual handling

ok300 added 9 commits May 30, 2024 14:44
# Conflicts:
#	lib/bindings/src/breez_liquid_sdk.udl
#	lib/core/Cargo.toml
#	lib/core/src/bindings.rs
#	lib/core/src/model.rs
#	lib/core/src/sdk.rs
#	packages/react-native/android/src/main/java/com/breezliquidsdk/BreezLiquidSDKMapper.kt
#	packages/react-native/ios/BreezLiquidSDKMapper.swift
#	packages/react-native/src/index.ts
@ok300
Copy link
Contributor Author

ok300 commented May 31, 2024

I saw you added the set_log_stream to the ignore list in the RN codegen, but you also need a add the manual handling

@dangeross done: 80b3b77

Copy link
Collaborator

@dangeross dangeross left a comment

Choose a reason for hiding this comment

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

Look good, I checked over all the RN changes. I think just the method naming is in question?

@ok300
Copy link
Contributor Author

ok300 commented May 31, 2024

Thanks @dangeross . Yes, only the method signature is open (name and arg type).

Return a LiquidSdkError::Generic instead of Anyhow error when initializing log stream on Dart bindings
@ok300
Copy link
Contributor Author

ok300 commented May 31, 2024

@dangeross @erdemyerebasmaz I updated and re-generated the bindings, can you please re-check if all looks good?

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

LGTM

@ok300 ok300 merged commit f6082f2 into main May 31, 2024
0 of 5 checks passed
@ok300 ok300 deleted the ok300-logger branch May 31, 2024 18:01
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.

Add Logger (log listener) similar to the breez sdk
4 participants