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

SwiftLog Compatibility (Issue) #111

Closed
Sherlouk opened this issue Feb 8, 2022 · 4 comments
Closed

SwiftLog Compatibility (Issue) #111

Sherlouk opened this issue Feb 8, 2022 · 4 comments
Labels

Comments

@Sherlouk
Copy link

Sherlouk commented Feb 8, 2022

Hi there 👋

swift-log is a package by Apple which allows for extensive logging within applications and packages. I'd love to continue using this but route errors through to this Diagnostics package - the best of both worlds!

In order to do this, we can create a custom "LogHandler" and simply route the data through to the existing DiagnosticsLogger. Fabulous.

Two issues at the moment though:

  1. file/function are expected to be of type "StaticString" but Swift Log simply uses "String". It would be nice to change the signature here to allow for more flexibility in where the data comes from.
  2. the error type requires conforming to Error which is not always possible (well, isn't required by Swift Log). To get around this I've created a custom DiagnosticsError struct which accepts the contents of the SwiftLog message payload which does get around this but it would be awesome if the package allowed us to simply provide a string and potentially even metadata*.

Support for metadata in logging messages would be handy. I'd expect this to be something that is shown to the developer if they tap on that line in the HTML report generated. It would then show a simple table with key/value pairs allowing for more context on the error.

Hope this makes sense!

@AvdLee
Copy link
Contributor

AvdLee commented Feb 9, 2022

Interesting thought, it would not be bad to make Diagnostics more flexible in this regard!

file/function are expected to be of type "StaticString" but Swift Log simply uses "String". It would be nice to change the signature here to allow for more flexibility in where the data comes from.

Dang, we just moved away from a regular String! It shouldn't be hard to revert it back I guess. Though, have you tried creating a static string instead from what you get out of swift-log? If it's not possible, would it make sense to log this as file/function or should we create a different metadata destination for these?

the error type requires conforming to Error which is not always possible (well, isn't required by Swift Log). To get around this I've created a custom DiagnosticsError struct which accepts the contents of the SwiftLog message payload which does get around this but it would be awesome if the package allowed us to simply provide a string and potentially even metadata*.

Interesting, never thought about this scenario, but it makes sense to allow sending Strings as errors and make them appear as such. Feel free to contribute and make this possible!

Support for metadata in logging messages would be handy. I'd expect this to be something that is shown to the developer if they tap on that line in the HTML report generated. It would then show a simple table with key/value pairs allowing for more context on the error.

This is an interesting idea, but the doubt I have is discoverability. I'm not sure yet how this should work. Do you have an example or inspiration you got this from?

@Sherlouk
Copy link
Author

Sherlouk commented Feb 9, 2022

Dang, we just moved away from a regular String! It shouldn't be hard to revert it back I guess. Though, have you tried creating a static string instead from what you get out of swift-log? If it's not possible, would it make sense to log this as file/function or should we create a different metadata destination for these?

It's not possible to get a StaticString from the swift-log library, for the time being I've simply prefixed the string into the message so it's like "MyFile.swift:42 - Some message has been set oh hello".

Interesting, never thought about this scenario, but it makes sense to allow sending Strings as errors and make them appear as such. Feel free to contribute and make this possible!

💯 Still playing around with the implementation and the app at large but will 100% come back round to this.

This is an interesting idea, but the doubt I have is discoverability. I'm not sure yet how this should work. Do you have an example or inspiration you got this from?

Not really, just kind of what I would maybe expect given the data available from Swift Log. I'm happy to raise a separate PR to show a bit of an example and we can take the conversation from there!

@github-actions github-actions bot added the Stale label Mar 12, 2022
@AvdLee
Copy link
Contributor

AvdLee commented Apr 7, 2022

A fix is on its way: #118

@WeTransfer WeTransfer deleted a comment from github-actions bot Apr 7, 2022
@AvdLee AvdLee removed the Stale label Apr 7, 2022
@github-actions
Copy link

github-actions bot commented May 8, 2022

This issue is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label May 8, 2022
@AvdLee AvdLee closed this as completed May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants