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

Logger infrastructure for SwiftUI #36

Closed
wants to merge 1 commit into from

Conversation

pauljohanneskraft
Copy link

Logger infrastructure for SwiftUI

♻️ Current situation & Problem

Fixes #9 by allowing the injection of Loggers into the SwiftUI environment. It doesn't (yet) make use of them, since I'm not sure on the desired granularity level of the subsystem and category values.

⚙️ Release Notes

  • Allows the injection of Logger instances into subviews to more easily define subview's logging behavior, especially for generic views.

📚 Documentation

  • Adds documentation for LoggerKey (even though it is private). I noticed that the other environment keys are internal, is there a specific reason for them not to be private?

✅ Testing

No testing code provided.

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@pauljohanneskraft pauljohanneskraft self-assigned this May 7, 2024
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for starting to work on this @pauljohanneskraft!

I think looking at #9 I would suggest to add some more functionality for this that the extension provide convenient mechanisms to inject their own Logger instance for all of their subviews (e.g., providing better defaults for the whole app, or something like SpeziAccount would want to inject it's own logger for all of its subviews). I could see that we might want to create a convenient view modifier that only requires a subsystem to inject a new logger in the lower environment?

Would be great to also get @Supereg's input on this as he was the original author of the issue.

It would be great to add some testing code and documentation around this as well. If we have some elements that currently using a logger (e.g. ValidationEngine) that could maybe benefit from this?


import OSLog
import SwiftUI

Copy link
Member

Choose a reason for hiding this comment

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

We generally add two spaces between an import statement and the first declaration.

Suggested change

private struct LoggerKey: EnvironmentKey {
static let defaultValue: Logger = .init()
}

Copy link
Member

Choose a reason for hiding this comment

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

Same element as above, we generally separate two types with two spaces.

Suggested change

///
/// This might be useful to provide a logger to generic subviews depending on the context they are being used in.
private struct LoggerKey: EnvironmentKey {
static let defaultValue: Logger = .init()
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if we should provide a default subsystem and category for the logger to be easily identifiable across logging output. E.g. a edu.stanford.spezi subsystem and views category?

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking about this as well, but I decided against it, since the property is simply called logger, so the association with Spezi might not be obvious which makes the default app-specific Logger object more intuitive imho. I don't have a strong opinion though, so if you do, I will change it.

@Supereg
Copy link
Member

Supereg commented May 15, 2024

I could see that we might want to create a convenient view modifier that only requires a subsystem to inject a new logger in the lower environment?

I kinda like the idea of a dedicated view modifier (like logger(subsystem:category)) but at the same time am not quite sure if embracing standard SwiftUI mechanisms is the better route (e.g., .environment(\.logger, Logger(subsystem: "...", category: "...")).

If we have some elements that currently using a logger (e.g. ValidationEngine) that could maybe benefit from this?

I think a lot of components inside SpeziViews can benefit from this. For example, the AsyncButton and other views.

@PSchmiedmayer
Copy link
Member

@Supereg Good point; doesn't really add a benefit to create new modifier just to add this small synthetic sugar that might be harder to understand. Looking at this it might not be worth expanding the API surface just for this.

Good point, might be good to see this tested and applied here @pauljohanneskraft to validate the API design within this package itself.

@pauljohanneskraft
Copy link
Author

Logging isn't really used in this package that much in general (I found a static logger in ValidationEngine and regular print statements in Tests), so I'm not sure how to apply it really, since ValidationEngine even seems to be a case, where using the Environment isn't necessarily the most elegant solution (since we would somehow need to inject that logger into ValidationEngine and Logger isn't Equatable, so we can't easily observe when it changes).

About SpeziAccount: It currently has its own implementation of a LoggerKey and it is only used in a way that an app can inject a custom Logger, but doesn't create its own Logger (except for the default value), so I kind of see this as the app being able to inject a Logger to the used views - not the views defining a new logger for its own subviews.

I think looking at #9 I would suggest to add some more functionality for this that the extension provide convenient mechanisms to inject their own Logger instance for all of their subviews (e.g., providing better defaults for the whole app, or something like SpeziAccount would want to inject it's own logger for all of its subviews).

This comment kind of goes the other way, which is why I'm mentioning this. I currently see this feature as Spezi purely reading the logger from the environment and the app primarily setting the logger. What do you think?

@PSchmiedmayer
Copy link
Member

@pauljohanneskraft @Supereg Wondering how we should progress about this PR and/or if we should close it.

It might rather make sense to add a logging Module in Spezi that would be environment accessible and could be used to generate derived loggers for Spezi modules and possibly allows access in a SwiftUI environment using the Spezi Module mechanism?

@Supereg
Copy link
Member

Supereg commented Aug 5, 2024

@pauljohanneskraft @Supereg Wondering how we should progress about this PR and/or if we should close it.

It might rather make sense to add a logging Module in Spezi that would be environment accessible and could be used to generate derived loggers for Spezi modules and possibly allows access in a SwiftUI environment using the Spezi Module mechanism?

I think there are two different reasons for improved Logger support in the SwiftUI context that were mentioned in the original issue description:

  • There are small, reusable and composable views (like AsyncButton would be) which are typically owned by their parent view and may reuse their logger. While we mention views from SpeziViews as example, there aren't any views that would actually use that as, e.g., AsnycButton doesn't log anything. If we would add support for that, doing a simple logger environment key would probably be the best idea here. However, I'm more and more leaning towards not supporting that as a) we do not really have a use case ourselves and b) if we cannot provide an automatic injection of a Logger it don't see people doing manual injection a lot.
  • The other use case is that views want to easily access the Logger of a Module they are linked to. If they are tied to a EnvironmentAccessible module, they might just access the Module via the @Environment property and directly access the logger property. The question is, should we support a mechanism that doesn't require a Module to be environment accessible for that? Something like a @Logger(MyModule.self) var logger DynamicProperty that basically uses the approach @PSchmiedmayer explained under the hood.

@PSchmiedmayer
Copy link
Member

Thanks for the context @Supereg!
I think the last option is one to consider, but I have been thinking a bit about this and I am not sure if this is worth the infrastructure surrounding it without a few clear use cases where we ran into issues that made this impossible. Ideally, larger-scale logic that requires a logger should be moved into modules or view models anyways that could constrain their own infrastructure.

As long as the different loggers in the Spezi ecosystem use a similar edu.stanford.spezi prefix, it is also easy to filter them out in the logging console, which should also make this an "easy" mechanism not really requiring infrastructure from our side.

If a developer really wants to access a logger from a module, they could then access it using the environment and access its logger instance (but arguably probably just call a function of the module). I would therefore lean toward not adding something specific around this for now, just pointing toward using a logger instead of print statements within our documentation and maybe even as a hint in SpeziViews.

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.

Easily accessible logger infrastructure for SwiftUI views
3 participants