-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fixed IsLogFromCurrentProject to include all severities #1062
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I have a couple of remarks.
There seem to be build failures caused by the changes.
Please also update the cleaner test cases by adding the corresponding log output.
src/logging.cc
Outdated
cleaned_base_filename.pop_back(); | ||
int dot_pos = cleaned_base_filename.find_last_of('.'); | ||
cleaned_base_filename.erase(dot_pos + 1, std::string::npos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do actually need to modify the string. Something like this should be sufficient:
cleaned_base_filename.pop_back(); | |
int dot_pos = cleaned_base_filename.find_last_of('.'); | |
cleaned_base_filename.erase(dot_pos + 1, std::string::npos); | |
const string::size_type dot_pos = cleaned_base_filename.find_last_of('.', cleaned_base_filename.size() - 1); |
src/logging.cc
Outdated
bool found_file = false; | ||
for (const auto& severity : LogSeverityNames) { | ||
string cleaned_base_filename_with_severity = cleaned_base_filename + severity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool found_file = false; | |
for (const auto& severity : LogSeverityNames) { | |
string cleaned_base_filename_with_severity = cleaned_base_filename + severity; | |
```suggestion | |
const string base_filename = cleaned_base_filename.substring(0, dot_pos); | |
bool found_file = false; | |
for (const auto& severity : LogSeverityNames) { | |
string cleaned_base_filename_with_severity = base_filename + severity; |
692680d
to
4e016e5
Compare
I'm not sure how I should modify cleaner test cases unfortunately |
You can extend any of the glog/src/cleanup_immediately_unittest.cc Lines 62 to 64 in ac12a9e
by additionally logging at the missing severities. Please note that the unit tests need to be updated either way because these are failing now with your changes. |
4e016e5
to
6ea4aea
Compare
Updated unit test and all checks should pass now hopefully |
Unfortunately, the cleanup tests are still failing. |
6ea4aea
to
71a04a1
Compare
Fixed an issue different way |
The unit tests are still failing though. |
Closing as abandoned. Please let me know if you want to continue working on this PR. |
Previously when executing following code LogCleaner would only delete overdue Error log files:
And if we changed the order to for example:
LogCleaner would only delete overdue Info log files and overdue Error and Warning files were ignored.
To include all severities during LogCleaner's sweep I tested filepath for all severities in a IsLogFromCurrentProject function.