-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix/logging arguments parsing #337
base: master
Are you sure you want to change the base?
Conversation
func log(type: OSLogType, log: OSLog?, _ message: String) { | ||
// empty implementation | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
а зачем пустая имплементация?
var mutableMessage = message | ||
|
||
mutableMessage.withUTF8 { (buf: UnsafeBufferPointer<UInt8>) in | ||
buf.baseAddress!.withMemoryRebound(to: CChar.self, capacity: buf.count) { str in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
force unwrap плохо
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В стандартной реализации тоже force unwrap. Видимо в данном случае можно 🤷♂️
public struct DefaultLoggerHandler: LogHandler { | ||
|
||
public var logInfo: OSLog | ||
public var logger: LogOutputRepresentater? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
с неймингом LogOutputRepresentater
явно что-то не то. хотя бы опечатка, но я бы ещё поменял на что-то более чётко отражающее суть
@available(iOS 14.0, *) | ||
extension Logger: LogOutputRepresentater { | ||
public func log(type: OSLogType, log: OSLog?, _ message: String) { | ||
self.log(level: type, "\(message)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
а нам вообще нужно \()
использовать? не можем напрямую отправить строку?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не можем там в качестве аргумента OsLogMessage и он скастить не может
public func log(_ message: StaticString, log: OSLog? = nil, type: OSLogType, _ arguments: CVarArg...) { | ||
let stringMessage = String(format: "\(message)", arguments: arguments) | ||
self.log(type: type, log: log, stringMessage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
правильно ли я понимаю, что в этом моменте мы всю секьюрность os_log теряем?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Как я понял, мы ничего не теряем, ведь конечная реализация логера - стандартная реализация os_log
https://github.com/apple/swift/blob/swift-5.2-branch/stdlib/public/Darwin/os/os_log.swift
Передача CVarArgs для StaticString
Теперь передача нескольких аргументов работает без багов:
Вывод будет таким:
Так же теперь работает string interpolation