-
Notifications
You must be signed in to change notification settings - Fork 55
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
quick and dirty fix to issue #45 #52
quick and dirty fix to issue #45 #52
Conversation
since this is an officials best practice/paperwork requirement, I am adding this as a stopgap while I work on converting the penalty code definition to something based on a runtime-loaded file and less copy-and-paste terrible (like an "is expellable" flag on a penalty definition).
updateMap.put("PenaltyCode.FO_EXP(EXP-D)", "Expulsion-Direction"); | ||
updateMap.put("PenaltyCode.FO_EXP(EXP-P)", "Expulsion-Illegal Position"); | ||
updateMap.put("PenaltyCode.FO_EXP(EXP-N)", "Expulsion-Interference"); | ||
updateMap.put("PenaltyCode.FO_EXP(EXP-G)", "Expulsion-Misconduct"); |
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.
Should we add unknown too? It seems unlikely to come up, but it's possible.
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 would add unknown for the sake of it
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.
Yeah, it's worth it for consistency I think.
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.
Can you add it to your pull so when I merge or Jared does we can capture it?
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 can create a quick PR after you merge for the sake of simplicity. My PR is big enough as-is.
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.
Actually, better idea. #62 contains both @official-sounding's commit and this change so you can merge or rebase that one, and close this one.
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.
Approve to Merge pending Jared.
…and-unknown PR #52 with unknown expulsion code added
fixes Issue #45 by adding EXP-* codes to the FO_EXP list of penalties, with a subset of penalties that are expellable.
Since this is an officials best practice/paperwork requirement, I am adding this as a stopgap while I work on converting the penalty code definition to something based on a runtime-loaded file and less copy-and-paste terrible, like an "can expel" flag on a penalty definition.