-
Notifications
You must be signed in to change notification settings - Fork 15
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
Bug daps 1243 datafed web logging #1251
Conversation
Reviewer's Guide by SourceryThis pull request reverses the order of the LogLevel enum to match the severity of the log levels, with CRITICAL being the most severe and TRACE being the least severe. Class diagram showing updated LogLevel enumerationclassDiagram
class LogLevel {
<<enumeration>>
CRITICAL: 0
ERROR: 1
WARNING: 2
INFO: 3
DEBUG: 4
TRACE: 5
}
note for LogLevel "Log levels reordered by severity:
CRITICAL (most severe) = 0
TRACE (least severe) = 5"
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @t-ramz - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Reversing the numeric values of log levels is a breaking change that could cause silent failures (link)
Overall Comments:
- Please provide justification for reordering the LogLevel enum values. This could break any code that depends on the numeric values. Have you verified there are no such dependencies?
- Several PR checklist items are incomplete, including the changelog entry. Please complete these before requesting review.
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
CRITICAL: 0, | ||
ERROR: 1, | ||
WARNING: 2, | ||
INFO: 3, | ||
DEBUG: 4, | ||
TRACE: 5, |
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.
issue (bug_risk): Reversing the numeric values of log levels is a breaking change that could cause silent failures
While aligning with Unix conventions (lower numbers for higher severity), this change could break existing code that relies on numeric comparisons or persisted log levels. Consider either keeping the original values or implementing a migration strategy that ensures backward compatibility.
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.
Based on the immediately following logic, the logging was already broken. This is a bug fix.
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.
Thank you for your feedback, we will generate fewer comments like this in the future.
Closes #1243
PR Description
Quick fix for the first part of linked ticket
Tasks
Summary by Sourcery
Bug Fixes: