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 support for continents and their structure #176

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Add support for continents and their structure #176

wants to merge 14 commits into from

Conversation

toadle
Copy link

@toadle toadle commented May 13, 2015

  • continents and sub-continents
  • Only EN and DE are currently supported locales

@ecbypi
Copy link
Contributor

ecbypi commented May 13, 2015

A few thoughts I wanted to ask about / bring up:

  • Why JSON for the territories information? Where does it come from? Is there anyway to store this as YAML the same way we store existing information? I'd prefer not to have to manage two sources of information and formats.
  • What's the use-case for this? Not to say there isn't one, more that I want to make sure I understand the problem you're aiming to solve.
  • Instead of inserting continents alongside countries, it make more sense to nest countries under the continents they belong to. It's a little weird for Carmen::World to have to look for the Carmen::Continent in order to return its subcontinents.

@toadle
Copy link
Author

toadle commented May 13, 2015

@toadle
Copy link
Author

toadle commented May 18, 2015

@ecbypi I'm happy to change some things, if this functionality is wanted in carmen. Like moving data to YAML and doing some refactoring.

Any feedback from you?

@ecbypi
Copy link
Contributor

ecbypi commented May 18, 2015

Sorry @toadle, it's been a hectic week.

If converting to YAML is possible, that would be great. Additionally, if the contains key could be represented as an array in YAML instead of a space-separated string that we'd have to process at runtime.

Regarding the nesting, I think it might be confusing to have two representations of the world via Carmen::World and Carmen::Continent as well as to groups of nesting that are still connected. It takes some extra time to process how a country is associated with a continent and vice versa when approaching these changes with the existing structure of data carmen uses (one-way nesting). If the information the JSON is built from is available online, it could be used in the update_data.rb script to properly nest the data. This would be a backwards-incompatible change, so we could consider it for v2.0.

I'm all for adding this functionality to the gem and thanks for taking the time to work on it and submit a PR.

@toadle
Copy link
Author

toadle commented May 29, 2015

I just converted the JSON data to YML, as suggested and removed the world-representation from the regular Continent-API. I would love to have the world only available through Carmen::World but that should be done with changing the data-loading structure. Which BTW is a bit beyond me, cause I don't understand how that is excatly done and IF the territoryContainment should be overridable.

The used source-data btw has recently been made available through a Github-project: https://github.com/unicode-cldr/cldr-core/tree/master/ . Could indeed server as a update_data.rb-source.

Is the plan the somehow merge this before changing the data-structure or what would be the plan?

@toadle
Copy link
Author

toadle commented Jul 23, 2015

@ecbypi Do we want to continue this somehow?

@ecbypi
Copy link
Contributor

ecbypi commented Jul 23, 2015

I do! My schedule with work has been very full for a while now. I'll squeeze in some time this weekend.

@zarelit zarelit mentioned this pull request Mar 27, 2017
@TylerRick
Copy link

Does someone want to try to rebase and merge dpaluy#1 ?

@toadle
Copy link
Author

toadle commented Dec 28, 2022

Wow this thing still lives 😇
The project i built it for has since died 😎

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