Extraction code validates, but tests fail

[stratofax]

Since this PR stayed in draft mode for a few days, the original description is no longer comprehensive, but retained here as an archive of the original post.

I tested the extraction code by updating ballotmaker validate, which accepts a JSON EDF and prints the results to the console, thus recapitulating @ion-oset 's edf-to-ballot-data.py script.

Unfortunately, when I tried to use the code in data as a library, I received some unexpected errors.

Here's a code snippet from demo_ballots.py with the errors listed as comments:

    extractor = BallotDataExtractor()
    election_data = extractor.extract(data)
    # Expecting election_data to be an ElectionData object, but
    # this produces an AssertionError
    assert isinstance(election_data, ElectionData)
    # this results in an AttributeError:
    # AttributeError: 'list' object has no attribute 'name'
    print(election_data.name)

[stratofax]

Note that changes to ballotmaker validate cause all tests to fail.

[ion-oset]

The code snippet is incorrect, and the existing code is right: it should be returning a list of ElectionData. The reason is explained in the comment block for Extractor._elections:

# In most cases there isn't more than one 'Election' in a report, but the
# standard allows more than one, so handle them [as a list of ElectionData]

The snippet should be:

extractor = BallotDataExtractor()
election_data = extractor.extract(data)
assert isinstance(election_data, list) and len(election_data) == 1 and isinstance(election_data[0], ElectionData)
election_data = election_data[0]
print(election_data.name)

Edit: I'll update my example in the forthcoming README and in #112 .

[stratofax]

Also check your function's return type hint

[ion-oset]

Yes, full type hint returns are already in my branch for the next round.

[stratofax]

I changed this to Draft while I do additional work.

[stratofax]

Local pytest still fails for me:

FAILED tests/test_cli.py::test_demo - ValueError: I/O operation on closed file.

Yet all checks pass on GitHub actions.

[stratofax]

Switched from Draft to Ready To Review because:

• All checks have passed!
• Generated ballots posted for review, too, on The latest ballots discussion
• The command ballotmaker demo is fully functional.

[ion-oset]

Looks great! The code is much easier to follow now that it's settling down.

I've noted a number of issues for your review, but all of them can be dealt with later if you choose. None of them block merging.

[stratofax]

@ion-oset Because I want to merge this PR onto development while it's very close to the code that produced the ballots I posted last Friday, I've used the following approach to address your excellent and helpful comments:

• If a suggested change was very simple, I implemented it (basically, one line or less)
• If a code change was slightly more complex, involving multiple lines or imports, I noted the change in the code with a TODO comment, and also included a backlink in the code to the review item here on this PR.
• If the review raised an issue that requires additional discussion or consideration, I linked out to a discussion for that purpose.

I then re-tested the code locally and re-generated the ballots just to ensure I didn't break anything. Now it's time to merge!