September ballot and human-readable IDs #106
Conversation
Significant update of contests: - Include '@id's of ballot measure contests (candidates already happening). - Include '@id's of candidate and ballot measure contest selections. - Updates and bug fixes for write-ins in candidate contests. - Multi-candidate slates: - Combine names of candidates into list. Names are stored in lists even when there is only one candidate. - Combine parties of candidate IFF they are the same. Parties are always stored in lists, and empty if there is no party.
Only updates the JSON file not the custom data file. See: https://github.com/trustthevote-project/nist-1500-100-103-examples/test_cases/september_test_case.json
- Candidate parties are now a list. - 'write_in' -> 'is_write_in'
These are NOT intended to follow any standard, merely to aid in debugging. There's an ongoing conversation about the format of human readable IDs.
|
@stratofax This PR addresses the improvements to the internal ballot that we discussed last week. It should address the additional features you requested on #86. The ordering is provided by the September test case added in NIST-1500-100-103 examples #66. I grafted in that test case and my other generator-based ballot changes to date and added human-readable IDs. If you approve of what you see, I can also add test cases for all jurisdictions beyond Spacetown. But let me know what you think before I do that. |
| Drop either field from result if it isn't present. | ||
| """ | ||
| # Note: party ID is returned to allow de-duplicating parties in callers. | ||
| id_ = candidate.party_id |
There was a problem hiding this comment.
Here's my chance to be a pedantic Pythonista! I'm not going to let this chance pass me by...
The trailing underscore is (by convention) a way to avoid namespace collisions with Python keywords. Since "id" is not a Python keyword, there's no need for a trailing underscore.
>>> import keyword
>>> print(keyword.kwlist)
['False', 'None', 'True', 'and', 'as', 'assert', 'async', 'await', 'break', 'class', 'continue', 'def', 'del', 'elif', 'else', 'except', 'finally', 'for', 'from', 'global', 'if', 'import', 'in', 'is', 'lambda', 'nonlocal', 'not', 'or', 'pass', 'raise', 'return', 'try', 'while', 'with', 'yield']
>>>
There was a problem hiding this comment.
Since all tests passed, I think we're OK to merge, unless you generated a ballot that doesn't look right.
It looks OK to me, but I didn't scrutinize it. Ideally take a look too before you merge.
Other things to review before you merge:
- Verify that this particular patch looks right to you. As you saw I made some layout changes in the data and
candidate.partyis now a list of party dictionaries. Just want to be sure I didn't miss something obvious.... - I think Data and layout updates to complete milestone 1: flat file PDF #105 needs to be merged before September ballot and human-readable IDs #106. It looks like this PR thinks the Updates to hit milestone 1 is part of it, and it shouldn't be.
There was a problem hiding this comment.
Since "id" is not a Python keyword, there's no need for a trailing underscore.
It's not a keyword but id is a builtin function and it can be clobbered so that you can't use it in the scope of the variable. It's best to treat it as a keyword and not risk the collision.
There was a problem hiding this comment.
Per our conversation merging is deferred until after milestone 1
There was a problem hiding this comment.
I stand corrected on id_. Well played!
I think I'll use more descriptive names for my ids, like candidate_id, to avoid name collisions. But I'm now totally on board with id_
|
@ion-oset please retarget this PR to the new |
There was a problem hiding this comment.
My question about multiple parties and candidates possibly not matching up is either a) irrelevant because I don't understand the code; or b) not relevant for what we need to do right now because we don't have the data to test it. If that's the case, let's post an Issue and move on. Either way, I think this is good to go!
| @@ -63,17 +63,20 @@ class CandidateData: | |||
| _names: list = field(init=False, repr=False, default_factory=list) | |||
| party: str = field(init=False) | |||
| party_abbr: str = field(init=False) | |||
| write_in: bool = field(init=False) | |||
| is_write_in: bool = field(init=False) | |||
There was a problem hiding this comment.
Much better than my original name!
| - Collects candidate parties into an array. | ||
| - If all candidates in a race share a single party they are combined into | ||
| one entry in the array. | ||
| - If any candidates differ from the others, parties are listed separately. |
There was a problem hiding this comment.
Does this mean that multiple parties and names are linked by the index in their respective lists? If so, what if one or more candidates don't have a party but others do?
This is an edge case that we don't need to deal with just yet, and it's hard to see how it works without some sample data. Perhaps this should be an Issue?
There was a problem hiding this comment.
I agree that this doesn't get worked out until we have real samples to work with, and can be its own issue. But I did play around with a couple of valid examples, so I'll answer your questions just to have a record of how I though it should work.
Multiple parties would be linked by their location in their respective lists yes.
The parties list is one of:
- Empty. All the candidates have the same party: no party.
- One party. All the candidates share it.
- More than one party. All candidates have their own party. The candidate lists and party lists have to have the same length.
If looping over the code the empty case is a trivial case of the multiple case. It's only the "single party shared by a ticket" that's a special case.
If so, what if one or more candidates don't have a party but others do?
The party for the candidates with "no party" should appear as as an empty dictionary. That's chosen so that checks on its contents (if field_name in party) will be legal but return False.
This works if the number of candidates and the number parties in the EDF match up. If they don't match the mismatch won't throw an exception in this code, but it will leave it as a special case for the caller. That needs fixing.
| name = candidate_name(candidate) | ||
| if name: | ||
| names.append(name) | ||
| party, _party_id = candidate_party(candidate, index) |
There was a problem hiding this comment.
We know we'll get a candidate ID but do we know we'll always get a party, if multiple parties are listed?
There was a problem hiding this comment.
Yes, but as noted above those parties are empty dictionaries. See candidate_party for details.
|
@ion-oset I've re-targeted this PR to the If you're OK with the comments, please go ahead and merge, or I can merge tomorrow morning. I noticed that it was easier to review after I merged the eariler PR #105 onto |
Done. I left some answers to your question about parties, but resolving that can wait - as you say we don't have data that tests that case.
Done. I've merged.
Yes. That's why it's better not to have the same change on multiple PRs. It will usually work, but it can be confusing. |
|
@ion-oset thanks! |
Use the September test case to get a conventional order for contests and candidate selections.
Also adds human-readable IDs in the last change. As we've discussed this is only intended to make debugging easier, not a formal proposal of a human-readable ID scheme.