Added Post-Name Change Compatibility Handlers, Added JavaDocs to PersonnelRole.java #4842
Conversation
…onnelRole.java Added compatibility handlers for the roles that hand their names changed by the name change project. Added JavaDocs for better code documentation and readability. Added a Nullable and Deprecated annotation.
IllianiCBT
added
Bug
Personnel
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4842 +/- ##
============================================
- Coverage 10.31% 10.30% -0.01%
+ Complexity 5881 5871 -10
============================================
Files 944 944
Lines 131420 131394 -26
Branches 19061 19063 +2
============================================
- Hits 13552 13545 -7
+ Misses 116535 116532 -3
+ Partials 1333 1317 -16 ☔ View full report in Codecov by Sentry. |
|
|
||
| // <50.1 compatibility |
There was a problem hiding this comment.
The actual changes start here
There was a problem hiding this comment.
As I think about this (and other similar code), when was the conversion from Magic Numbers to Enum strings done? If pre-0.49.19, yank it.
There was a problem hiding this comment.
I'm fairly confident it was pre-49.19. I'm going to keep this in, though, because users updating from 50 are still going to run into the issue this addresses and for the sake of a few extra lines of code it maintains cross milestone compatibility. I absolutely agree with removing it once we hit our next compatibility threshold.
| MMLogger.create(PersonnelRole.class) | ||
| .error("Unable to parse " + text + " into a PersonnelRole. Returning NONE."); | ||
| .error("Unable to parse {} into a PersonnelRole. Returning NONE.", personnelRole); |
There was a problem hiding this comment.
The MMLogger doesn't actually have a method that will do this. Will need to use a String.format() otherwise it'll pop up a dialog.
There was a problem hiding this comment.
Oh that is really good to know. I'll pop in a correction real quick.
Is there any way to add this functionality to MMLogger, because IDEA recommends doing what I did here and it's going to be a bit of a pain having to remember to check devs aren't following IDEA's recommendation whenever we review PRs
There was a problem hiding this comment.
Would require a re-write of those MMLogger methods to go from error to errorWithDialog (and related) which isn't a bad idea over all. It would allow mimicking the Log4J commands but would require a re-write in the few places that actually use the current method for a dialog first.
There was a problem hiding this comment.
Thanks for the heads up, btw. I was at a loss why I was getting a dialog box whenever I ran the personnel tests, but had assumed it was because I was testing in a non-headless environment.
Should be fixed now :)
Replaced string formatting with concatenation for error logging in `PersonnelRole`. This change was necessary due to the absence of support for string formatting in the current logging mechanism.
Added compatibility handlers for the roles that had their names changed by the name change project.
Added JavaDocs for better code documentation and readability.
Added a Nullable and Deprecated annotation.
Closes #4833