-
Notifications
You must be signed in to change notification settings - Fork 1
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
Testing and reports #54
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.
Looks good
|
||
# Folder within the ONS Postcode Directory archive holding names and codes files | ||
names_codes_root = DATA_ROOT / self.settings["ONS Names and codes folder"] |
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.
did you mean to remove DATA_ROOT?
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.
Yes I think so as data/
is in settings.json
already https://github.com/the-scouts/geo_mapping/blob/4c6484d041d7a1ebc0fca964d714b68504d80faf/settings.json#L5-L6
|
||
# Folder within the ONS Postcode Directory archive holding names and codes files | ||
names_codes_root = DATA_ROOT / Path(self.settings["ONS Names and codes folder"]).resolve() |
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.
did you mean to remove DATA_ROOT?
Couple of things in this one
Move testing to parameterised using hypothesis. Shouldn't be any noticable difference, but this will be helpful for identifying edge cases etc as we move forwards (and increase test coverage!)
Add functionality relating to Clear workflow for supporting custom shapefiles #51. I'm still not happy with this as (1) it requires regenerating both the merged census extract and the reduced ONS file, and (2) it doesn't preserve population sizes etc. (2) is an easier fix, for (1) the ideal is to dynamically hook in without any pre-setup etc.
WIP for now as not ready for merging