CLDR-18927 non-TC orgs should tally by count, not just latest time #5248
Conversation
- non-TC orgs count the number of voters, not the time
- add tests - improve (reduce) test noise - fix some cases where VoteResolver getters haven't updated votes - un-deprecate getOrgVote() - it is used by the vorting machinery! Added javadoc notes for clarity.
btangmu
left a comment
btangmu
left a comment
There was a problem hiding this comment.
Some follow-up to fix the TODOs, comments and naming would be ideal. The Counter object seems to have been intended for more generic usage, while "participants" seems to belong in a more specific class.
| } | ||
|
|
||
| /** return the number of times participate() was counted */ | ||
| public final int getParticipants() { |
There was a problem hiding this comment.
It's a little unclear what this means, just from the function name and description. It could be the number of people who participated, or the number of times a particular person participated, or the sum of the latter for all people...
There was a problem hiding this comment.
this is the raw counter. So participants is meant to be a generic term here, per the javadoc.
| return participants; | ||
| } | ||
|
|
||
| /** add one to the participant coutn */ |
| */ | ||
|
|
||
| // temporarily make the top voted value this org's value. | ||
| // TODO: may not be needed, see below |
There was a problem hiding this comment.
"TODO" comment should have a ticket number; this PR completes this ticket
| considerTime = items.getTime(considerItem); | ||
| } else if ((time > maxtime) && (count == maxCount)) { | ||
| considerCount = items.getCount(considerItem); // TODO: == maxCount == count? | ||
| considerTime = items.getTime(considerItem); // TODO: == maxtime == time? |
| annotateTranscript( | ||
| "--- %s vote is for '%s' with strength %d", | ||
| org.getDisplayName(), considerItem, considerCount); | ||
| // TODO: is this ever not reached if there is a value? |
| public class TestVoteResolver { | ||
|
|
||
| @Test | ||
| void testNonTcOrgByCountNotTime() { |
| public long value; | ||
| private final int forceUnique; | ||
| public long time; | ||
| public int participants; |
There was a problem hiding this comment.
possibly not, all of the other data fields are public though.
|
2 participants
CLDR-18927
ALLOW_MANY_COMMITS=true