Skip to content
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

Add import script for NHS England CCG areas #192

Closed
wants to merge 4 commits into from

Conversation

chris48s
Copy link
Contributor

As with the Police Force boundaries (see pull request #73), NHS CCG boundaries for England are available as open data, but they aren't formatted in such a way that they import nicely into MapIt.
This import script solves that problem, importing the CCG boundaries cleanly.
This may be useful to pull into master for users self-hosting, or could be useful for mapit uk if you decide to publish the CCG boundaries (as raised in issue #74).

@dracos
Copy link
Member

dracos commented May 22, 2015

I realise this copies the previous PR you mention, but is there a reason this command isn't a subclass of the mapit_import Command? I can't obviously see a reason why it has to use call_command and duplicate a lot of options/set up code, but I may well have missed something.

@chris48s
Copy link
Contributor Author

Commits have been tidied as requested. I'll have a go at refactoring this based on your suggestions and get back to you with an updated pull request.

@chris48s
Copy link
Contributor Author

I've refactored this to extend mapit_import, as requested. I've constructed an appropriate option_list by slicing mapit_import's option_list tuple. This solution is more succinct then redefining it but the downside to this is that adding or removing elements from the start or in the middle of mapit_import's option_list tuple would require updating the line:

    option_list = mapit_import.Command.option_list[:7] + \
                  mapit_import.Command.option_list[8:11] + \
                  mapit_import.Command.option_list[15:20]

accordingly.
Just something to be aware of there.

@dracos
Copy link
Member

dracos commented Jun 19, 2015

Thanks, that's a bit less code :) A suggestion for your downside - see how what I've done in 3175d64 uses create_parser to set defaults instead, doing it that way (or other similar things, perhaps remove_option) might save you having to do a slice, and be more robust.

@chris48s
Copy link
Contributor Author

No problem - updated as requested.

@torotil
Copy link
Contributor

torotil commented Jun 22, 2019

I think this is outdated as the current CCG boundary files can be imported with the generic import command.

@chris48s
Copy link
Contributor Author

Agreed 👍 Closing this and tidying up some other forgotten PRs that went nowhere

@chris48s chris48s closed this Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants