Verbose logging #1027
Verbose logging #1027
Conversation
This is a brilliant idea @ollieatkinson |
What about hiding color and formatting from users. You might wonder why? If we allow colouring and formatting things from the API, we might end up with inconsistent formatting, and that's a bad experience for the user. I think we can compare this to design systems. The reason why they exist is because they ensure we can create consistent design experiences across all the products that we build with them. The same applies here. We could have a consistent styling experience by providing a set of formatting tokens/components for users to use. But if we hide colours and styles, how do users highlight things in the messages that they are logging? What I'd do is try to recognise patterns in the messages that we output. For example, we might find that developers very often include command suggestions in their outputs:
We could define how a command should be formatted (e.g. in white and bold), and then do it internally. The API could be something along the lines of: logger.info("Run \(.command("tuist generate")) to run the generate the project before building it.") |
|
Dumping another idea here. What about adding a guidelines to the contributors documentation about the grammar that we should use when logging information to the user?
This is a bit picky, but consistent styling with our messages is also a great experience for the users. |
| @@ -4,7 +4,7 @@ import PackageDescription | |||
|
|
|||
| let package = Package( | |||
| name: "tuist", | |||
| platforms: [.macOS(.v10_11)], | |||
| platforms: [.macOS(.v10_12)], | |||
There was a problem hiding this comment.
This is a breaking change that we need to agree with. Since the latest version is 10.15, I think it's ok dropping support for 10.11. What do you think @tuist/core ?
There was a problem hiding this comment.
By upping from 10.11 to 10.12 we would loose support for Xcode 7
|
Beautiful @ollieatkinson β€οΈ. Have you thought about a testing strategy that we could follow if we want to test that we get the right output? Having a global variable makes the API very convenient, but I think it complicates testing a bit. |
Yes, since we have decoupled logging and the output we would just need to register a logging handler which we are able to ciphen messages out of. LoggingSystem.bootstrap(TestingLogHandler.init)public struct TestingLogHandler: LogHandler {
static var collected: [Logger.Level: [String]] = [:]
public var logLevel: Logger.Level
public let label: String
public init(label: String) {
self.label = label
self.logLevel = .trace
}
public func log(
level: Logger.Level,
message: Logger.Message,
metadata: Logger.Metadata?,
file: String, function: String, line: UInt
) {
Self.collected[level, default: []].append(message.description)
}
public var metadata = Logger.Metadata()
public subscript(metadataKey key: String) -> Logger.Metadata.Value? {
get { metadata[key] }
set { metadata[key] = newValue }
}
} |
There was a problem hiding this comment.
Nice work @ollieatkinson π
Having standardised logging will be a great addition! I like the three loggers you included, they were handy to test out this branch.
Some thoughts about colours & printing ... βοΈ
I think we can compare this to design systems. The reason why they exist is because they ensure we can create consistent design experiences across all the products that we build with them. The same applies here. We could have a consistent styling experience by providing a set of formatting tokens/components for users to use
Agree on this one.
I'd add, if we're to unify logging and printing, styling can be treated as metadata as opposed to getting applied to the message contents directly - for example when using the console app logger, the terminal styling characters aren't desired in the messages.
Having it as independant metadata would enable outputting logs to multiple destination if needed (e.g. console, console app, and a file) where the styling is applied only where needed and using the appropriate format for the output.
One aspect I am going back and forth on is logging vs printing - I can see a lot of similarities but also some differences. Seeing Tuist is a command line tool, printing to the console is effectively its view layer so to speak where styling matters. There could even be instances where you'd always want to print to the console regardless of logger choice for example:
tuist --helptuist versiontuist dump
Having looked at the changes in this pull request, for the majority of cases I can see trying to make the distinction of "should I print? or log? or do both?!" to be quite tricky π - as such leaning more in favour of your approach of unifying the APIs as there aren't that many use cases where printing is distinctly required over logging.
|
@ollieatkinson your pull request is missing a changelog! |
Codecov Report
@@ Coverage Diff @@
## master #1027 +/- ##
==========================================
- Coverage 78.84% 77.26% -1.59%
==========================================
Files 238 242 +4
Lines 11346 11580 +234
==========================================
+ Hits 8946 8947 +1
- Misses 2400 2633 +233
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks great overall - some minor comments. Thanks for pushing this through @ollieatkinson
One of the acceptance tests failed, I suspect its due to the error output format changing:
features/step_definitions/shared/fixtures.rb:5
Then tuist generate yields error "Error: Manifest not found at path ${ARG_PATH}" # features/step_definitions/shared/tuist.rb:29
The output error message:
Manifest not found at path /var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/d20200305-5178-exn279
Does not contain the expected:
Error: Manifest not found at path ${ARG_PATH}
(Minitest::Assertion)
./features/step_definitions/shared/tuist.rb:41:in `/tuist generate yields error "(.+)"/'
features/generate.feature:41:in `Then tuist generate yields error "Error: Manifest not found at path ${ARG_PATH}"'
Resolves #39
Short description π
Replace
Printerforswift-logand expose a command line argument for--verboseallowing for more logs to be printed to the console.Solution π¦
swift-logis the official swift logging framework, it was originally developed to remove the descrepency between all of the different server-side logging frameworks. Even though we are not a server-side application we do take logging seriously, and it makes sense for us to be able to integrate with the standard tools provided by apple.swift-logprovides a really nice abstraction away from logging so that the thing logging is agnostic of where the log is output. Currently I have written three implementations:noticeand it will provide colorisation and a minimalistic view of the log - no time, no file, no metadata.os.logAPI, useful if you want to trace the logs using Console.appEach Tuist module will have an internal
loggervariable which can be used to log, the label is used in the detailed log so that you have more information about which module the log message was generated.All logs are collected and dispatched to the handler which has been bootstrapped, this can either be one of the handlers I have defined, or multiple of them at once. We may in the future decide we want to have a handler which writes to disk, which might help you track metrics of the tool over time.
At the call-site when wanting to create a log you would use the standard
swift-logAPI, the function name is the same as the level of the log. For example this is a log with levelwarning:In the
StandardLogHandlersome assumptions have been made when it comes to colourising your message.These colours can however be overriden if you want by using one of the following API's:
β I would appriciate feedback because the colourisation I am least precious about and I do not have a strong opinion on.
TODO π©βπ»π¨βπ»
notice