-
Notifications
You must be signed in to change notification settings - Fork 88
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.
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
[UK] October 2017 Boundary-Line import. #304
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand the ins and outs of importing new Boundary Line data, but the inline comments are ++helpful in explaining what's going on. The code looks good to me, except for maybe adding a bit of exception handling as noted (or perhaps I've misunderstood the circumstances where this code is run and that's actually not needed at all).
👍
mapit_gb/controls/2017-10.py
Outdated
|
||
|
||
def check(name, type, country, geometry, ons_code, commit, **args): | ||
"""Should return True if this area is NEW, False if we should match""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this comment be expanded to describe why it would return an Area instead of True/False?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, that comment is a bit out of date, the one in 2016-05.py is better!
mapit_gb/controls/2017-10.py
Outdated
# May 2017 Boundary-Line gave Clydebank Central the wrong ID, | ||
# return the relevant row if present | ||
if type == 'UTW' and ons_code == 'S13003126': | ||
return Area.objects.get(codes__type__code='gss', codes__code='S13003136') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be wrapped in a try
/except
like the ones below, or will it definitely always exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the area doesn't exist, it'll raise an exception to create a new area, which is what we want and what would happen anyway (as the same thing would happen when it tried to look up S13003126).
# These areas were incorrectly named in May 2017 Boundary-Line, | ||
# so find the misnamed entries if present | ||
if type == 'CED' and name == 'Uckfield North ED': | ||
return Area.objects.get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto the try
/except
here, in case (for whatever reason) someone's skipped May 2017?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these were new in May 2017 (hence them getting their names wrong), so if May 2017 isn't present, same as above, a not exists exception will be raised, as it would have been anyway a bit later on.
These haven't been used for many years now.
7714039
to
3616fcd
Compare
And tweak find_parents so it will find the ward parents before this script is run.
These were removed as options in Mapit here: mysociety/mapit#304
These were removed as options in Mapit here: mysociety/mapit#304
Hopefully comments in control file explain things (need expanding if not!).
Fixes mysociety/mapit.mysociety.org#64
Fixes #219