Dev #156
Dev #156
Conversation
- only used in this file - does nothing reasonable - tests pass
- get_list_id should have one function - getting the derived value is another function
|
|
||
| def get_candidate_id(line): | ||
| return str(int(line.knr)) | ||
| def get_number_of_mandates(line): |
There was a problem hiding this comment.
I tend to follow GO's dictum these days. For setter functions use set_, for getter functions omit the get_. I think we should adopt that in all new code.
There was a problem hiding this comment.
Agree. In this case I left it since otherwise var names conflict with function names and the following routine gets harder to read.
| col = 'ausmittlungsstand' | ||
| try: | ||
| t = int(line.ausmittlungsstand or 0) | ||
| assert 0 <= t <= 3 |
There was a problem hiding this comment.
You shouldn't use assertions and exceptions like this. Exceptions are for unexpected things (a value being invalid is expected in an import). Assertions on the other hand are for developers only. The end-user should never see the result of an assertion.
Here, you could simply use a condition, then raise a ValueError:
if not (0 <= t <= 3):
raise ValueError( _('Value ${col} is not between 0 and 3', mapping={'col': col})))Of course you would have to do that outside the current try block (or you would catch your own exception). But that would be clearer anyway and it would make obvious that you are not returning the t anywhere in the function 😉
I would also rename t into something a bit more lengthy, like maybe result.
| if not hasattr(line, col): | ||
| raise ValueError(_('Missing column: ${col}', mapping={'col': col})) | ||
| number = line.listnr or '0' | ||
| number = '999' if number == '99' else number # blank list |
There was a problem hiding this comment.
I'm not quite sure what # blank list means. Could you elaborate your comment here? Why is 99 turned into 999?
There was a problem hiding this comment.
Blank lists are lists without a candidate or those with a missing party. WabstiC uses code 99 for those, but they map to 999 that represents the same from other interfaces. This can read in open_data_de.md. I made the comment more clear referencing open_data_de.md.
| @@ -549,6 +614,7 @@ def import_election_wabstic_proporz( | |||
| if election.domain == 'region' and districts and election.distinct: | |||
| if principal.has_districts: | |||
| if len(districts) != 1: | |||
| # FIXME: better error messages | |||
There was a problem hiding this comment.
Here it would make sense to also elaborate. Why can't you write a better error message at this time?
There was a problem hiding this comment.
At the moment I do not understand the difference between the two cases, so a better FIXME Message would be "Separate error msg for the two cases". I still lack the overview to write a good and valid error message.
There was a problem hiding this comment.
Alright, then I would just elaborate. It's okay to write long FIXME's and to really write down your current thoughts and what you don't get yet. Otherwise the FIXME will be very cryptic to yourself when you stumble upon it in the future :)
| raise ValueError(_('Missing column: ${col}', mapping={'col', col})) | ||
| if '.' in line.knr: | ||
| return line.knr.split('.')[0] | ||
| # replaces int(int(line.knr) / 100)) |
There was a problem hiding this comment.
Is this the previous function in place? If so, you can just remove it. Only leave the current code when changing functions. It is hard enough to understand old code when you look at it in a year, without having to look at even older code.
If you want to look at even older code, git log / git blame is better.
There was a problem hiding this comment.
Remove the old code hint, old code is not used anywhere anymore.
I wrote new wrappers for the error handling using new translations strings. The translations have not been updated yet.