coderholic / django-cities

Countries and cities of the world for Django projects
MIT License
921 stars 375 forks source link

Broken import because of merged tqdm progress bars #112

Closed psviderski closed 8 years ago

psviderski commented 8 years ago

Recently merged pull reques #111 breaks import of countries:

Traceback (most recent call last):
  File "./manage.py", line 53, in <module>
    execute_from_command_line(sys.argv)
  File "/Users/spy/.virtualenvs/edy/lib/python3.4/site-packages/django/core/management/__init__.py", line 354, in execute_from_command_line
    utility.execute()
  File "/Users/spy/.virtualenvs/edy/lib/python3.4/site-packages/django/core/management/__init__.py", line 346, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/spy/.virtualenvs/edy/lib/python3.4/site-packages/django/core/management/base.py", line 394, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/Users/spy/.virtualenvs/edy/lib/python3.4/site-packages/django/core/management/base.py", line 445, in execute
    output = self.handle(*args, **options)
  File "/usr/local/Cellar/python3/3.4.1_1/Frameworks/Python.framework/Versions/3.4/lib/python3.4/contextlib.py", line 30, in inner
    return func(*args, **kwds)
  File "/Users/spy/.virtualenvs/edy/lib/python3.4/site-packages/cities/management/commands/cities.py", line 96, in handle
    func()
  File "/Users/spy/.virtualenvs/edy/lib/python3.4/site-packages/cities/management/commands/cities.py", line 215, in import_country
    for item in tqdm([d for d in data if d['code'] not in no_longer_existent_country_codes],
  File "/Users/spy/.virtualenvs/edy/lib/python3.4/site-packages/cities/management/commands/cities.py", line 215, in <listcomp>
    for item in tqdm([d for d in data if d['code'] not in no_longer_existent_country_codes],
NameError: name 'no_longer_existent_country_codes' is not defined

The variable no_longer_existent_country_codes here https://github.com/coderholic/django-cities/commit/173094a3af08428b4d6223ea025e82634f9ddd56#diff-2aa026c5281a0419bdb351a98b1ef545R215 is not defined. @blag: I have no idea why we should care about obsolete country codes while importing a new countryInfo.txt dump.

coderholic commented 8 years ago

Thanks for flagging. @blag can you look into this? If not I can take a look soon.

blag commented 8 years ago

Presumably the list of countries will be shown to users at some point, so I tried to ensure that countries that don't exist anymore (but are still present in countryInfo.txt) should not be imported, because it would be pointless to allow users to select countries that don't exist. However, I could envision somebody wanting to add historical information to previously-existing countries, so I wanted to make it an option that was overridable in Django's settings.

I have been thinking that it would be a better solution to use django-model-utils's TimeFramedModel and a default manager that excludes countries that no longer exist. This will allow the most flexibility but it does require information on when countries with ISO 639-1 codes were formed/began (default: -infinity datetime, unless we have that information) and dissolved/conquered/ended (default: infinity datetime).

@coderholic: PR #113 fixes this by importing a user-overridable option from cities.conf. What do you think of the TimeFramedModel idea?

@psviderski: This is fixed in PR #113. So far just the Netherlands Antilles (AN - dissolved into territories in 2010) and Serbia and Montenegro (CS - split into separate countries in 2006: ME and RS) are still listed in countryInfo.txt but don't actually exist anymore. Those non-existent countries are ignored when importing unless overridden in your settings. Is this reasonable behavior for your use case? Or is there something we can do better for you?

Edit: Typos. Typos everywhere.

blag commented 8 years ago

After this is merged I've got PR #114 in the pipe.

psviderski commented 8 years ago

@blag: Thank you very much for the detailed clarification of the purpose of no-longer-existed countries. Everything sounds reasonable. Thanks for the quick fix as well, good job!