Initial Work on MMLogger with Sentry Tracking #5702
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5702 +/- ##
============================================
- Coverage 29.09% 29.09% -0.01%
Complexity 13898 13898
============================================
Files 2506 2507 +1
Lines 265989 266000 +11
Branches 47567 47569 +2
============================================
Hits 77393 77393
- Misses 184677 184688 +11
Partials 3919 3919 ☔ View full report in Codecov by Sentry. |
| * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY | ||
| * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * for more details. | ||
| */ |
There was a problem hiding this comment.
This is not the copyright notice from our wiki page.
https://github.com/MegaMek/megamek/wiki/MegaMek-Coding-Style-Guide
There was a problem hiding this comment.
Also, indentation is supposed to be 4 spaces...
| import io.sentry.Sentry; | ||
|
|
||
| public class MMLogger { | ||
| MMLogger() { |
There was a problem hiding this comment.
just wondering... why not make the constructor private (this is really not important)
There was a problem hiding this comment.
Didn't think about it at the time.
| public static void info(String message) { | ||
| Logger logger = LogManager.getLogger(); | ||
|
|
||
| if (logger.isInfoEnabled()) { |
There was a problem hiding this comment.
this suggests that you plan to replace all current info() calls with this one, right? Because it doesnt make sense to check if info is enabled for some but not all info logs. In that case, would it make sense to really replace all logger calls? And if so, we'd need to have an error() call without an exception parameter because not all errors come from an exception.
There was a problem hiding this comment.
That is the intention and I can add the other error method.
But the number of places that would need to be adjusted for this, I'm not going to do it all at once and only a few files at a time. A slow migration to this system.
There was a problem hiding this comment.
I misphrased that. It's not that I expect you to actually do any replacing, I only meant how things would ideally be. Mass-replacing will only create merge conflicts. As there is no intolerable downside to keeping LogManager calls, we can leave it to whoever touches classes to switch to MMLogger.
There was a problem hiding this comment.
Yea, my intention is to do 5-10 files at a time for Sentry and just replace the logs as I go to keep it uniform. It'll take a few months just for MM a few more for MML and a few more for MHQ. I figure by the time we reach the start of J21 testing I'll finally be done adding this in everywhere.
|
I"ll add the JavaDoc when I make the rest of the changes. |
Spaces: 2 -> 4 JavaDoc Added non-exception Error methods. Privatised constructor
Initial work on adding a central MMLogging class to handle log writing, Sentry reporting, and Panel display.