FileRotationLoggerDelegate Fix Spelling #34
Conversation
There was a problem hiding this comment.
Thanks for your PR and fixing typo(FileRotationLoggerDeletate --> FileRotationLoggerDelegate) 🙂
As for the log rotation suffix , I intentionally use only numeric suffix in order to match the general log rotation format on Linux OS(without txt extension)....
So, could you revert only this commit(c34356e8e3879bb39656edd1b11622497ef6f290)?
Thanks.
| let rotatedFileURL = oldArchivedFileURL | ||
| .deletingPathExtension() | ||
| .appendingPathExtension("\(generationNumber)") | ||
| .appendingPathComponent(".txt") |
There was a problem hiding this comment.
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)...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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? | |||
Codecov Report
@@ Coverage Diff @@
## main #34 +/- ##
=======================================
Coverage 95.83% 95.83%
=======================================
Files 11 11
Lines 408 408
=======================================
Hits 391 391
Misses 17 17
Continue to review full report at Codecov.
|
This reverts commit c34356e.
|
/danger |
No description provided.