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

ocd for 'November 2019 Spanish general election' and existing parliament #173

Merged
merged 7 commits into from
Oct 17, 2019
Merged

ocd for 'November 2019 Spanish general election' and existing parliament #173

merged 7 commits into from
Oct 17, 2019

Conversation

sguenther85
Copy link
Contributor

No description provided.

identifiers/country-es.csv Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@
ocd-division/country:es/autonomous_region:ce,Ceuta
ocd-division/country:es/autonomous_region:ml,Melilla
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd lean towards "region" here instead of "autonomous_region" but I won't block on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i used "autonomous_region" because of the portugal PR.
Where we have a difference between i.e Lisboa and Acores

ocd-division/country:pt/region:li - Lisboa
ocd-division/country:pt/autonomous_region:ac - Acores
Lisboa is a region in Portugal, but Acores is a autonomous region (no normal region).

So I'd suggest to use "autonomous_region" in this special cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see Ceuta and Melilla listed as autonomous cities on the Wikipedia page about autonomous communities, which suggests to me that they are autonomous communities like the other 'states'. Shouldn't they then all have the same OCD type?

I got confused because your comment is about Portugal, but this PR is about Spain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changes both now to "autonomous_city".
Sorry for confusion. I used portugal as an example for spain, because of the similar problem.
Ceute and Melilla have in spain an other "political status". So my suggestion would be to have a difference to the other communities, but if you both say "we should use the same ocd type", I can change that again.
@jpmckinney @jdmgoogle

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the details for Portugal, so I defer to you, @sguenther85.

identifiers/country-es/comunidad_autonoma.csv Show resolved Hide resolved
identifiers/country-es/provincia.csv Outdated Show resolved Hide resolved
multi-line issue
removed last line
@sguenther85
Copy link
Contributor Author

sguenther85 commented Oct 16, 2019

@jdmgoogle @jpmckinney
it is possible to continue here?

@jpmckinney any question or opinions about @jdmgoogle and my questions above?
@jdmgoogle any opinions on my las comment to your review-changes?

The elections are not far way. Would like to move forward and find a good solution ;)

Copy link
Contributor

@jdmgoogle jdmgoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. Just need @jpmckinney to weigh in.

@@ -0,0 +1,2 @@
ocd-division/country:es/autonomous_region:ce,Ceuta
ocd-division/country:es/autonomous_region:ml,Melilla
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM.

identifiers/country-es/comunidad_autonoma.csv Show resolved Hide resolved
identifiers/country-es/provincia.csv Outdated Show resolved Hide resolved
identifiers/country-es/comunidad_autonoma.csv Show resolved Hide resolved
@@ -0,0 +1,2 @@
ocd-division/country:es/autonomous_region:ce,Ceuta
ocd-division/country:es/autonomous_region:ml,Melilla
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see Ceuta and Melilla listed as autonomous cities on the Wikipedia page about autonomous communities, which suggests to me that they are autonomous communities like the other 'states'. Shouldn't they then all have the same OCD type?

I got confused because your comment is about Portugal, but this PR is about Spain.

identifiers/country-es/provincia.csv Outdated Show resolved Hide resolved
Pronvices with iso code sorted
autonomous_community with iso codes
autonomous_region to autonomous_city
@jpmckinney jpmckinney self-requested a review October 17, 2019 15:18
Copy link
Member

@jpmckinney jpmckinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for the changes.

@jpmckinney jpmckinney merged commit 903e122 into opencivicdata:master Oct 17, 2019
@sguenther85 sguenther85 deleted the feature/country-es branch October 18, 2019 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants