WeTransfer / Diagnostics

Allow users to easily share Diagnostics with your support team to improve the flow of fixing bugs.
MIT License
933 stars 53 forks source link

Use String instead of StaticString to be more inline with swift-log behaviour #118

Closed antranapp closed 2 years ago

antranapp commented 2 years ago

I am wrapping DiagnosticsLogger inside a LogHandler from swift-log package so that I can use swift-log as the main logger for my app.

But currently, Diagnostics is using StaticString for #file, #line, #function in the log method

swift-log is using String instead of StaticString for #file, #line, #function

I think, changing StaticString to String would make it more interoperable with swift-log and provides more flexibility to integrate with other libraries as well.

wetransferplatform commented 2 years ago
Messages
:book: View more details on Bitrise
:book: DiagnosticsTests: Executed 30 tests (0 failed, 0 retried, 0 skipped) in 0.511 seconds

Code Coverage Report

Name Coverage
Diagnostics 75.23% ⚠️

Generated by :no_entry_sign: Danger Swift against a6c107550adedf09c72f7b0611646aec3f9431cf

antranapp commented 2 years ago

Hi @BasThomas, I found this issue explaining why swift-log uses String instead of StaticString

For my use case, I am wrapping DiagnosticsLogger inside a LogHandler provided by swift-log: https://github.com/antranapp/DebugPane_Diagnostics/blob/master/Sources/DebugPane_Diagnostics/DiagnosticsLogHandler.swift

The LogHandler protocol requires the parameters to be String instead of StaticString

With this, I can automatically get all the log that I handle globally inside my app.

BasThomas commented 2 years ago

Awesome, thanks for that!

I wonder if we should treat this as a breaking change — if any packages depend on this one, and expect a StaticString, that may be breaking. Would it be possible in any way to have these as overloads until we'd cut a version 4.0?

AvdLee commented 2 years ago

I'll have a look at CI to make this PR green in the upcoming days!

wetransferplatform commented 2 years ago

Congratulations! :tada: This was released as part of Release 4.0.0 :rocket:

Generated by GitBuddy