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

Code Quality: Purge logfiles to only keep the last 100 lines #13115

Merged
merged 6 commits into from
Aug 6, 2023

Conversation

QuaintMako
Copy link
Contributor

Resolved / Related Issues

  • Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers.
    Closes Feature: Purging logs after a given size #12182

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?
    1. Made sure the process works when no logfile exists.

@QuaintMako
Copy link
Contributor Author

I'm opening this as a draft to have a bit of a discussion around the implementation. I've got several points:

  1. The current implementation is not the most efficient that there is. It's ready the collection of lines twice. It could done in one go, but I decided to go with this implementation for clarity's sake.

  2. The purge is done currently at the start of the project, increasing ever so slightly the time of start up. And it's not helped the first point above. Moving the purge at the closing of the app might be an idea? Reactivity is not a big deal at that point.

  3. The call to purge the logs is done at the moment during the constructor. I do not like it, but either I'm missing something big, or the actual implementation of the logger is really difficult to access. We could fix that by creating a new IFilesLogger that exposes the purge method, and implement the ILogger interface.

Any thought?

@yaira2
Copy link
Member

yaira2 commented Aug 1, 2023

The purge is done currently at the start of the project, increasing ever so slightly the time of start up. And it's not helped the first point above. Moving the purge at the closing of the app might be an idea? Reactivity is not a big deal at that point.

Regarding this point, I don't think it's a problem if the task is delayed.

@yaira2 yaira2 changed the title Feature: Pure logfiles to keep last 100 lines on start Code Quality: Pure logfiles to only keep the last 100 lines on start Aug 1, 2023
@yaira2 yaira2 changed the title Code Quality: Pure logfiles to only keep the last 100 lines on start Code Quality: Pure logfiles to only keep the last 100 lines Aug 1, 2023
@QuaintMako
Copy link
Contributor Author

Regarding this point, I don't think it's a problem if the task is delayed.

This would require to have access to the PurgeLog method outside of the class, which is currently a problem. See point 3 above.

@yaira2
Copy link
Member

yaira2 commented Aug 2, 2023

We could fix that by creating a new IFilesLogger that exposes the purge method, and implement the ILogger interface

Let's try this

@QuaintMako QuaintMako marked this pull request as ready for review August 5, 2023 17:28
@QuaintMako
Copy link
Contributor Author

I ended up finding a way less complicated in the end to make the purge process async.

As a sidenote, for the culture, expanding ILogger would have been pretty hard because of the pretty complex process of generic logging we're using.

Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 merged commit 0eb2269 into files-community:main Aug 6, 2023
@yaira2 yaira2 changed the title Code Quality: Pure logfiles to only keep the last 100 lines Code Quality: Purge logfiles to only keep the last 100 lines Aug 6, 2023
@yaira2 yaira2 added the ready to merge Pull requests that are approved and ready to merge label Aug 6, 2023
@QuaintMako QuaintMako deleted the 12182_LogPurge branch August 31, 2023 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Purging logs after a given size
2 participants