Skip to content
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.

Sign up for GitHub

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

Jump to bottom

September ballot and human-readable IDs #106

Merged
merged 5 commits into from
Sep 8, 2022
Merged

September ballot and human-readable IDs #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
db0773e
Update of JSON ballot generator.
ion-oset Sep 5, 2022
fb195a0
Update Spacetown data to match generator updates.
ion-oset Sep 5, 2022
fe81fbc
Use September test case for Spacetown data.
ion-oset Sep 5, 2022
818f21b
Update the internal Spacetown data to use the September test case.
ion-oset Sep 5, 2022
9b91232
Switch to human readable IDs.
ion-oset Sep 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/electos/ballotmaker/ballots/contest_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better than my original name!

name: str = field(init=True, default="")

def __post_init__(self):
self.id = self._can_data.get("id", "")
self._names = self._can_data.get("name", [])
_party_dict = self._can_data.get("party", {})
_party_list = self._can_data.get("party", [])
assert 0 <= len(_party_list) <= 1, \
f"Multiple parties for a slate/ticket not handled: {_party_list}"
_party_dict = _party_list[0] if len(_party_list) == 1 else {}
self.party = _party_dict.get("name", "")
self.party_abbr = _party_dict.get("abbreviation", "")
self.write_in = self._can_data.get("write_in")
if self.write_in:
self.is_write_in = self._can_data.get("is_write_in")
if self.is_write_in:
self.name = "or write in:"
else:
for count, can_name in enumerate(self._names):
Expand Down
2 changes: 1 addition & 1 deletion src/electos/ballotmaker/ballots/contest_layout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ def __init__(self, contest_data: CandidateContestData):
" and ", "<br />and<br />"
)
# add line for write ins
if candidate.write_in:
if candidate.is_write_in:
candidate.name += ("<br />" * 2) + ("_" * 20)
contest_line = f"<b>{candidate.name}</b>"
if candidate.party_abbr != "":
Expand Down
101 changes: 68 additions & 33 deletions src/electos/ballotmaker/demo_data/ballot_lab_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,19 +84,67 @@ def candidate_name(candidate: Candidate):


def candidate_party(candidate: Candidate, index):
"""Get the name and abbreviation of the party of a candidate as it appears on a ballot."""
party = index.by_id(candidate.party_id)
name = text_content(party.name) if party else ""
abbreviation = (
text_content(party.abbreviation)
if party and party.abbreviation
else ""
)
result = {
"name": name,
"abbreviation": abbreviation,
}
return result
"""Get the name and abbreviation of the party of a candidate as it appears on a ballot.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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']
>>>

Copy link
Collaborator Author

@ion-oset ion-oset Sep 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per our conversation merging is deferred until after milestone 1

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_

party = index.by_id(id_)
name = text_content(party.name) if party else None
abbreviation = text_content(party.abbreviation) if party and party.abbreviation else None
result = {}
if name:
result["name"] = name
if abbreviation:
result["abbreviation"] = abbreviation
return result, id_


def candidate_contest_candidates(contest: CandidateContest, index):
stratofax marked this conversation as resolved.
Show resolved Hide resolved
"""Get candidates for contest, grouped by slate/ticket.

A slate has:

- A single ID for the contest selection
- Collects candidate names into an array.
- 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@ion-oset ion-oset Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Notes:
- There's no clear guarantee of a 1:1 relationship between slates and parties.
stratofax marked this conversation as resolved.
Show resolved Hide resolved
"""
# Collect individual candidates
candidates = []
for selection in contest.contest_selection:
assert isinstance(selection, CandidateSelection), \
f"Unexpected non-candidate selection: {type(selection).__name__}"
names = []
parties = []
_party_ids = set()
if selection.candidate_ids:
for id_ in selection.candidate_ids:
candidate = index.by_id(id_)
name = candidate_name(candidate)
if name:
names.append(name)
party, _party_id = candidate_party(candidate, index)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know we'll get a candidate ID but do we know we'll always get a party, if multiple parties are listed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but as noted above those parties are empty dictionaries. See candidate_party for details.

parties.append(party)
_party_ids.add(_party_id)
# If there's only one party ID, all candidates share the same party.
# If there's any divergence track them all individually.
if len(_party_ids) == 1:
parties = parties[:1]
result = {
"id": selection.model__id,
"name": names,
"party": parties,
"is_write_in": bool(selection.is_write_in)
}
candidates.append(result)
return candidates


def candidate_contest_offices(contest: CandidateContest, index):
Expand Down Expand Up @@ -136,21 +184,9 @@ def contest_election_district(contest: Contest, index):
def extract_candidate_contest(contest: CandidateContest, index):
"""Extract candidate contest information needed for ballots."""
district = contest_election_district(contest, index)
candidates = []
candidates = candidate_contest_candidates(contest, index)
offices = candidate_contest_offices(contest, index)
parties = candidate_contest_parties(contest, index)
write_ins = []
for selection in contest.contest_selection:
assert isinstance(
selection, CandidateSelection
), f"Unexpected non-candidate selection: {type(selection).__name__}"
# Write-ins have no candidate IDs
if selection.candidate_ids:
for id_ in selection.candidate_ids:
candidate = index.by_id(id_)
candidates.append(candidate)
if selection.is_write_in:
write_ins.append(selection.model__id)
result = {
"id": contest.model__id,
"title": contest.name,
Expand All @@ -159,14 +195,9 @@ def extract_candidate_contest(contest: CandidateContest, index):
# Include even when default is 1: don't require caller to track that.
"votes_allowed": contest.votes_allowed,
"district": district,
"candidates": [
{"name": candidate_name(_), "party": candidate_party(_, index)}
for _ in candidates
],
# Leave out offices and parties for now
"candidates": candidates,
# "offices": offices,
# "parties": parties,
"write_ins": write_ins,
}
return result

Expand All @@ -179,10 +210,14 @@ def extract_ballot_measure_contest(contest: BallotMeasureContest, index):
selection, BallotMeasureSelection
), f"Unexpected non-ballot measure selection: {type(selection).__name__}"
choice = text_content(selection.selection)
choices.append(choice)
choices.append({
"id": selection.model__id,
"choice": choice,
})
district = contest_election_district(contest, index)
full_text = text_content(contest.full_text)
result = {
"id": contest.model__id,
"title": contest.name,
"type": "ballot measure",
"district": district,
Expand Down
Loading