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.

Sign up for GitHub

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

FileRotationLoggerDelegate Fix Spelling #34

Merged
merged 3 commits into from
Jan 26, 2022
Merged

FileRotationLoggerDelegate Fix Spelling #34

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b47b027
spelling fix
LePips Jan 24, 2022
c34356e
log rotation to have txt file
LePips Jan 24, 2022
1f17426
Revert "log rotation to have txt file"
LePips Jan 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions Sources/Puppy/FileRotationLogger.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public class FileRotationLogger: FileLogger {
public var maxFileSize: ByteCount = 10 * 1024 * 1024
public var maxArchivedFilesCount: UInt8 = 5

public weak var delegate: FileRotationLoggerDeletate?
public weak var delegate: FileRotationLoggerDelegate?
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for fixing this!


public init(_ label: String, fileURL: URL) throws {
try super.init(label, fileURL: fileURL)
Expand All @@ -37,7 +37,10 @@ public class FileRotationLogger: FileLogger {
let oldArchivedFileURLs = ascArchivedFileURLs(fileURL)
for (index, oldArchivedFileURL) in oldArchivedFileURLs.enumerated() {
let generationNumber = oldArchivedFileURLs.count + 1 - index
let rotatedFileURL = oldArchivedFileURL.deletingPathExtension().appendingPathExtension("\(generationNumber)")
let rotatedFileURL = oldArchivedFileURL
.deletingPathExtension()
.appendingPathExtension("\(generationNumber)")
.appendingPathComponent(".txt")
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for your proposal, but I intentionally use only numeric suffixes here in order to match the general log rotation format on Linux OS(without txt extension)...

Copy link
Contributor Author

@LePips LePips Jan 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I apologize that this was in here, it wasn't intended to be here as instead work on my own fork. Should've done this spelling fix on a branch instead of my main!

I do notice this is Linux's version of file rotation, however I think it may be more beneficial to (or at least allow to) not follow that convention. The use of the original file declaration allows developers to use names like log.txt/log.log and this makes viewing logs on mobile in the Files app possible. With the Linux convention, it does not. So instead of appending the number index or UUID, I would personally want something that would result in log-1.txt (example, not final).

Just something that I noticed as I would need to view logs on device instead of sharing them to a computer for viewing.

If you are okay with this proposal of at least adding an option between Linux style or file suffix friendly, I am more than happy to do the work with necessary modifications when it comes time for review.

Copy link
Contributor Author

@LePips LePips Jan 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

At the same time, I think file rotation based on file size is either unfinished (work in progress) or not working as intended. It will only work on the first time that a max file size was reached, and then the original log file can grow without bound. At least with my own experimentation with a 10kb file max to easily test with.

Copy link
Owner

@sushichop sushichop Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just something that I noticed as I would need to view logs on device instead of sharing them to a computer for viewing.

Thank you for sharing your idea.
I agree that we should add a new option to keep the file extension(e.g. log, txt) in order to give more options for users. I may add this option myself when I spare time🙂
Before that, I will merge the PR to fix typo!

At the same time, I think file rotation based on file size is either unfinished (work in progress) or not working as intended.

IMO, I assume it depends on the change about keeping file extension...
At least, we need to modify code more to rotate the file correctly while keeping the file extension.

debug("generationNumber: \(generationNumber), rotatedFileURL: \(rotatedFileURL)")
if !FileManager.default.fileExists(atPath: rotatedFileURL.path) {
try FileManager.default.moveItem(at: oldArchivedFileURL, to: rotatedFileURL)
Expand Down Expand Up @@ -113,7 +116,7 @@ public class FileRotationLogger: FileLogger {
}
}

public protocol FileRotationLoggerDeletate: AnyObject {
public protocol FileRotationLoggerDelegate: AnyObject {
func fileRotationLogger(_ fileRotationLogger: FileRotationLogger, didArchiveFileURL: URL, toFileURL: URL)
func fileRotationLogger(_ fileRotationLogger: FileRotationLogger, didRemoveArchivedFileURL: URL)
}
2 changes: 1 addition & 1 deletion Tests/PuppyTests/FileRotationLoggerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ final class FileRotationLoggerTests: XCTestCase {
}
}

extension FileRotationLoggerTests: FileRotationLoggerDeletate {
extension FileRotationLoggerTests: FileRotationLoggerDelegate {
func fileRotationLogger(_ fileRotationLogger: FileRotationLogger, didArchiveFileURL: URL, toFileURL: URL) {
print("didArchive! didArchiveFileURL: \(didArchiveFileURL), toFileURL: \(toFileURL)")
}
Expand Down