Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update the logging properties to opt-out of the prefix events #844 #845
Update the logging properties to opt-out of the prefix events #844 #845
Changes from 5 commits
8b68f75
d3f2a2f
6d8f307
7822d6f
2e2d0e5
9ca8473
c9d5b78
54a7463
17219b7
0e9bb76
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@mickeyz07 -- IMO, this is a major implementation kludge to me. Generally, I was expecting in whatever static initializer that checks the value for the
Logger.LogPrefix
property, that any error / exception that would be thrown that you would just treat it as 'true' and then log something vialogSpecial
. If for some reason that approach turns out to be impossible, then the next best alternative would be to create a special dedicated getLogPrefix method. Another preferred alternative (something I've been meaning to do for a while now) is to create a newBoolean getBooleanProp(String propName, boolean default)
and use that, and just pass in 'true' for the default, so if the specified property name is not a boolean, it defaults to the specified default value (which should be true forLogger.LogPrefix.
I am also very confused why this is even written the way that it it; it appears that you could omit the entire:and just went with the specific property name check that follows. (And why this specific check? I would have expected
Logger.LogPrefix
here, since that's the new property. So whyLogger.LogEncodingRequired
? I'm confused.)@jeremiahjstacey and @xeno6696 - I'd like your opinion on this, but I don't like this at all. Can either of you think of any alternatives?
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.
I think I agree @kwwall - Updates in this file will lead to additional scope that I think we'll be better off not trying to tackle in this effort.
@mickeyz07 - My understanding of what the desire here is to try to solve the problem at the lowest level possible. I think I agree with the intent, but I think you can agree that it's not the cleanest or most maintainable due to current restrictions in the library's interfaces.
To facilitate a faster integration of this feedback, I would like to propose the following:
It's not the best possible solution, but I think that's the best we can do in the current library implementation.
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.
Okay, my thoughts, exactly, and what you described was my preferred approach at resolving this.
@mickeyz07 - Please make the change that @jeremiahjstacey detailed above. Thanks.
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.
Suggestion: Add blank line here for improved readability.