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

Unified Logging #888

Closed
wants to merge 1 commit into from
Closed

Unified Logging #888

wants to merge 1 commit into from

Conversation

schmeat
Copy link
Contributor

@schmeat schmeat commented Feb 1, 2024

Checklist

  • I have read CONTRIBUTING.md
  • I have described what this PR contains
  • This PR addresses one or more open issues that were assigned to me:
    - Unified Logging Solution #124
  • If this PR alters the UI, I have attached pictures/videos

Pull Request Information

About this Pull Request

This is just a proof of concept for logging right now, with minimal files edited. Want to get consensus before I modify every file with logging. @Sjmarf @EricBAndrews

Additional Context

N/A for now

private let logger: Logger

required init(file: String = #fileID) {
self.logger = Logger(subsystem: AppConstants.loggerSubSystem, category: file)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the category be the file name for now. Not sure if we should have defined categories for logging, such as "user interaction", "networking", etc. It may be a lot to maintain and enforce going forward if we do that.

@schmeat
Copy link
Contributor Author

schmeat commented Feb 1, 2024

It would be nice to have a linting rule to warn people using print to use MlemLogger instead. I'm not sure how to do that in current project since I don't see a swiftlint markup file.

I see the file in root, of course. I'll update the rule at the end.

@schmeat schmeat changed the title [Draft] Unified Logging Unified Logging Feb 1, 2024
@EricBAndrews EricBAndrews added this to the v1.3 milestone Feb 1, 2024
@EricBAndrews
Copy link
Member

I like the concept! I'm tentatively scoping this for 1.3--pending the face ID PR, we're in full bugfix mode for the 1.2 release.

Re: categorization, I think that would be a good idea--there's a lot of discussion to be had about what those categories should be, but the added dev time categorizing logs has in my experience always paid off eventually.

We might want to look into an architecture like this one, where categorized loggers are defined as static properties on the Logger class--that saves us from needing to dependency inject or instantiate loggers everywhere

@schmeat
Copy link
Contributor Author

schmeat commented Feb 1, 2024

Yeah makes sense with categorization. I didn't make it statically defined because I have the file name coming in through the init. Once we define the categories, I won't need to do that.

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