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

Logging implementation for mapping on Apple and Non-Apple OSs #21

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

stoicswe
Copy link

@stoicswe stoicswe commented May 24, 2024

Description

Attempt for a logging implementation that maps to either the standard Apple logger if building for an Apple-OS target or the SwiftLogger library if the target is a non-Apple OS

Linked Issues

#18

Type of Change

  • Bug Fix
  • New Feature
  • Documentation

Checklist:

  • My code follows the ATProtoKit API Design Guidelines as well as the Swift API Design Guidelines.
  • I have performed a self-review of my own code and commented it, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings or errors in the compiler or runtime.
  • My code is able to build and run on my machine.

Screenshots (if applicable)

Attach any screenshots or GIFs showcasing the changes effect.

Additional Notes

Add any other notes about the Pull Request here.

Credits

If you want to be credited in the CONTRIBUTORS file, you can fill out the form below. Please don't remove the square brackets.

  • Name: Nathaniel Knudsen
  • GitHub: stoicswe
  • Bluesky: stoicswe.com

@MasterJ93
Copy link
Owner

MasterJ93 commented May 25, 2024

Thanks for your help with this. I took a quick look at it and there seems to be some misunderstanding with respect to this.

First, as of right now, the issue has a "draft" tag, which means that it's still being worked on before others can consider tackling it. However, I'll leave this pull request in here. Once that tag goes away, people can begin to solve the issues.

Second, as stated in the Contributing Guidelines, you need to put your pull requests in the develop branch, not main. main is like a "beta" version of ATProtoKit, while the releases are the official versions, and develop is the "alpha." I want develop to deal with the bugs and ironing them out. I want main to add features one at a time before being put into the releases (unless it's a bug fix, which in this case, it needs to be added straight into main). It seems that this file isn't visible enough and so I'll work toward making that possible even more.

Third, and finally, the current file you've edited, Logging.swift, wasn't something I was considering when making the draft. The purpose of the draft was to add log events into the various files, since this was something the project has been lacking on. However, I wasn't able to find time to do this, which is why I made it. That said, I would encourage you to keep working on it, because if there are indeed improvements to the file, I'll take it into consideration. That said, I'll soon update the issue to mention this.

@stoicswe stoicswe changed the base branch from main to develop May 26, 2024 19:26
@stoicswe
Copy link
Author

Basic logging messages have been added to the classes across the library. The commits have been separated by a per-file basis to try to make the PR more reviewable. Please let me know if there are more areas to improve the logs.

@MasterJ93
Copy link
Owner

Anything other than ATProtoKit, ATProtoTools, and ATProtoAdmin classes should not be given log events yet, as they're still in development and I feel there will be massive changes in there, still. However, I forgot about adding ATProtocolConfiguration and ATFacetParser, which are both complete, so thanks for making those changes in there.

Other than that, this is looking good, I think! 👍

@stoicswe
Copy link
Author

Quick update on this PR, I have been busy the past week or so, but plan to get back to editing this likely tomorrow. Sorry about the delay to looking into this PR and getting the implementation completed.

@MasterJ93 MasterJ93 added the enhancement An enhancement to an existing feature. label Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement to an existing feature.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants