Wabstic import #158
Wabstic import #158
Conversation
- Passing tests - Removing archive search specific modifications
- Move into the else block, that gets executed if try does not raise any error
- test catches first of execpetions, the rest works the same way
There was a problem hiding this comment.
Looks good to me, aside from a few small things. You certainly want to avoid consuming all the CSV lines however - that's a blocker for me.
I think coercing None to 0 makes sense, as CandiateResult.votes wants it that way. When in doubt we probably want to stick with the existing model. A lot more thinking goes into building up the model than adding features later, so I guess we trust that it makes sense as is 😉
| def line_is_relevant(line, number, district=None): | ||
| if district: | ||
| return line.sortwahlkreis == district and line.sortgeschaeft == number | ||
| else: |
There was a problem hiding this comment.
You can remove the else, since the if above always returns:
if district:
return line.sortwahlkreis == district and line.sortgeschaeft == number
return line.sortgeschaeft == number| :param none_be_zero: raises ValueError if line.col is None | ||
| :return: integer value of line.col | ||
| """ | ||
| assert hasattr(line, col), 'Check done in load_csv' |
There was a problem hiding this comment.
What does 'Check done in load_csv' mean? Could you elaborate here?
If you need longer texts, you can also write assertions like this:
assert hasattr(line, col), f"""
{col} was not found on {line} - this should have failed in load_csv
"""There was a problem hiding this comment.
I forgot to drop this line, since in load_csv in common.py, checks if the expected columns are there is performed already. The comment was a remainder of that.
| return line.sortgeschaeft == number | ||
|
|
||
|
|
||
| def validate_integer(line, col, none_be_zero=True): |
There was a problem hiding this comment.
I'd say non_be_zero should be named treat_none_as_zero. What do you think?
There was a problem hiding this comment.
it's better like this, I also added a default kwarg to return that default and changed the lines where the votes are received for the candidate.
| entity_id = int(line.bfsnrgemeinde or 0) | ||
| def get_entity_id(line): | ||
| col = 'bfsnrgemeinde' | ||
| # try: |
There was a problem hiding this comment.
Commented out code should not be committed, unless it is meant to be activated in the near future (for example, if a Puppet deployment has been prepared and tested, but not yet rolled out).
| @@ -79,6 +81,16 @@ def import_election_wabstic_majorz( | |||
| entities = principal.entities[election.date.year] | |||
| election_id = election.id | |||
|
|
|||
| def has_no_lines(lines, filename): | |||
| if not list(lines): | |||
There was a problem hiding this comment.
You consume the whole list of lines, to check if there's at least one line. That seems wasteful. If lines is not a generator, you can just check for by running if lines.
If lines is a generator, don't have to consume it to check for it. You can do either this:
for line in lines:
return False # has lines
return True # has no linesOr by getting the next value of the generator with a fallback:
if next(lines, None):
return True # has lines
return False # has no lines
There was a problem hiding this comment.
Yes, it's definitively a blocker...
| @@ -179,6 +185,16 @@ def import_election_wabstic_proporz( | |||
| entities = principal.entities[election.date.year] | |||
| election_id = election.id | |||
|
|
|||
| def has_no_lines(lines, filename): | |||
| if not list(lines): | |||
There was a problem hiding this comment.
Same as above, this should not traverse all lines.
- unclear which files could be empty and must be importable - does not fit script flow causing errors anyway and aborting import
gewahlttogewaehlt. Fixed failing import of majorz election test dataset that raised "Missing column: gewahlt".