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

Country coder supports GB-SCT so use that directly #4256

Merged
merged 3 commits into from
Oct 26, 2020

Conversation

peternewman
Copy link
Collaborator

They also handle US-HI etc, so all those states could be dropped too.

@peternewman
Copy link
Collaborator Author

I've just seen you asked for this in rapideditor/country-coder#26 🤦

Do we want the other stuff to switch across too?

@peternewman
Copy link
Collaborator Author

Blocked by rapideditor/country-coder#29 and it not being released yet. Although nothing actually seems to be using Scotland that I can see...

@bhousel
Copy link
Member

bhousel commented Oct 5, 2020

Yeah I was going to say - I don't think country-coder actually supports this level of detail.

When we need sub-country detail, I've been just drawing a custom geojson and dumping them into the features/ folder
(and I think it would be wonderful for all the transit networks to have a geojson saying where they operate).

We do have a few of these in features/ already:

@peternewman
Copy link
Collaborator Author

Yeah I was going to say - I don't think country-coder actually supports this level of detail.

When we need sub-country detail

Erm England is a country:
https://en.wikipedia.org/wiki/England

(and I think it would be wonderful for all the transit networks to have a geojson saying where they operate).

I assume you mean where they currently operate, couldn't this be extracted from OSM and then verified, rather than people having to/trying to draw custom geojson, assuming you want a particular corridor it runs in or something?

Although it may not have its own ISO code at all levels.

We do have a few of these in features/ already:

I'd have thought, due to the risk of errors as below, having them in just one place would be better than having to roll our own.

This is currently wrong as it includes the Isle of Man (I had to look it up).

@bhousel bhousel added the waitfor Waiting for something before we can do this label Oct 5, 2020
@peternewman
Copy link
Collaborator Author

Are you onboard with this in principle generally @bhousel then I could delete the duplicate geojsons and potentially start fixing up the config pending the release of the new version?

@bhousel
Copy link
Member

bhousel commented Oct 6, 2020

Are you onboard with this in principle generally @bhousel then I could delete the duplicate geojsons and potentially start fixing up the config pending the release of the new version?

Yes 👍 Anything we can do to avoid shipping geojson, and just use whatever country-coder supports is a win..
I'd hold off doing the work until it's actually released though.

@peternewman
Copy link
Collaborator Author

I'd hold off doing the work until it's actually released though.

Okay, I sort of assumed that if they support a particular thing we can guarantee it will appear eventually so we could at least delete our geojson, even if it's not worth the conflicts of editing the presets yet?

@bhousel
Copy link
Member

bhousel commented Oct 6, 2020

Okay, I sort of assumed that if they support a particular thing we can guarantee it will appear eventually so we could at least delete our geojson, even if it's not worth the conflicts of editing the presets yet?

I wouldn't merge something like that. The locationSets would be invalid in the interim, and links would stop working on http://nsi.guide

@peternewman
Copy link
Collaborator Author

Yeah I didn't expect you to, just preemptively starting the assessment of what's duplicated and then hopefully the checks would flag the broken location sets to fix up.

@peternewman
Copy link
Collaborator Author

peternewman commented Oct 16, 2020

They've released ( https://github.com/ideditor/country-coder/releases/tag/v4.0.0 ), although just looking at package.json here do we need to get this bumped too? https://github.com/ideditor/location-conflation

peternewman referenced this pull request in rapideditor/location-conflation Oct 24, 2020
@bhousel bhousel removed the waitfor Waiting for something before we can do this label Oct 26, 2020
@bhousel
Copy link
Member

bhousel commented Oct 26, 2020

These are unblocked now with location-conflation 0.6 and country-coder 4.0 🎉

@bhousel bhousel merged commit de79cb9 into osmlab:main Oct 26, 2020
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