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

Replace os.log with swift-log #10

Merged
merged 1 commit into from
Nov 5, 2024
Merged

Replace os.log with swift-log #10

merged 1 commit into from
Nov 5, 2024

Conversation

cpageler93
Copy link
Contributor

Hey @ppeelen,

thank you for your work on this project!

I tried to use it in an executable within a swift package, but unfortunately, it crashed.
Bundle.main.bundleIdentifier is an optional value, because there is no bundle identifier in non-bundled applications.

The crash occurred because of the forced unwrap of bundleIdentifier in the initializer of the logger. First I thought about making the value for the parameter subsystem accessible from outside, but I think it would be nicer to have https://github.com/apple/swift-log as a logging system.

This approach would allow users of this package to decide where to log, by using log handlers (which can still be os.log).

What do you think about this approach? Would you be open to considering a transition to SwiftLog for flexibility in logging?

Copy link
Owner

@ppeelen ppeelen left a comment

Choose a reason for hiding this comment

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

Hi @cpageler93!
I think this is a great approach; yes it adds a dependency which I try to avoid, but I think it is OK in this case. I it a common package and a small one as well. The identifier one could be different, but I don't have any strong opinion regarding this.

Nice job and thanks for contributing!

@ppeelen ppeelen merged commit e05e4de into ppeelen:main Nov 5, 2024
1 check passed
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.

2 participants