-
Notifications
You must be signed in to change notification settings - Fork 374
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
NameError: global name 'VALIDATE_POSTAL_CODES' is not defined #148
Comments
I started by fixing the import in management/commands/cities.py.
However this leads to another issue:
Don't have deep enough understanding of Django to tackle the bug |
Fixed VALIDATE_POSTAL_CODES import issue. Now same issue: unique=True on slug field will not work also with cities because same names across differents countries/regions. |
@spout did you try removing the |
@chris-y-meyers removed unique=True from SlugModel, it works. |
@spout it worked for you ? |
@chris-y-meyers: all was successfully imported. |
This issue as well as #146 fixed in https://github.com/chris-y-meyers/django-cities. |
Is there a point to slugs if they aren't unique? |
@spout Slugs must be unique because their point is to be used as human-readable path part of URLs. If these generated slugs are not unique with the data I've picked then we need to pick different data to slugify for cities. |
@blag Yes removing the We need to think how to restructure the models. To say I will be further looking into this issue. |
Restructuring our models is definitely outside the scope of this PR, but I'm happy to review PRs for it. The problem with doing so is the import script needs to support all of the different model structures, and every restructure we do complicates the import script. In terms of model structure, there's no large differences between this and django-cities-light. They solve some of the same problems in different ways (swapping out models for custom models), and I like their I'm working on #147 to properly fix the unicode issue. And while the tests are passing, the import script is not (yet). |
Slugs most definitely don't need to be unique. Slugs for locations collide frequently, and even more so for postalcodes. That just means you need to add more information to the URL, such as the country slug too, to differentiate. For example, I'd much prefer to have something like: /us/newcastle rather than /newcastle-1 |
That makes sense - how should we handle |
Subregion slugs are not unique within their respective regions: django.db.utils.IntegrityError: duplicate key value violates unique constraint "cities_subregion_region_id_e7611eb7_uniq"
DETAIL: Key (region_id, slug)=(2239858, kiwaba-nzoji) already exists. where ALTER TABLE public.cities_subregion
ADD CONSTRAINT cities_subregion_region_id_e7611eb7_uniq UNIQUE(region_id, slug); Both of these subregions are in the same country, the same region, and have the same name: http://www.geonames.org/2240688/kiwaba-nzoji.html This means that both of them will have the same slug. @coderholic Guidance please. |
Yes, you can't add any unique constraints here at all. If you do want a completely unique identifier then you need to use the ID. When you want to use a slug you'll need to use application logic to do what you want to do (eg. either pick one based on largest population in the very rare case of a duplicate, or use the id and the slug in the url - this is pretty common, NNNNN-slug - or whatever makes sense in your application) |
All, I have added slugs to all models in PR #147 (as well as fixing this issue, adding more tests, and fixing the unicode encoding issue). It is possible for users to define their own global slugify function that takes the object and the default slug data: def custom_slugify(obj, slug_data):
...
return modified_slug_data that will allow users to specify their own slugify semantics. A Users can specify their own slugify function by setting the (Edit: @ALL definitely doesn't work, so I'm at-mentioning you all by name to get your attention) If #147 looks good to all of you I'll merge it into |
Thanks for the work @blag I appreciate that setting the slug is now customizable.
|
@chris-y-meyers Thanks for the feedback. I've had issues with those two slugs in the past; I fixed their slugify logic and added them to the test data so we don't break them in the future. I've also removed the unique attributes on the slugs in migration If you don't mind testing these new changes again I would greatly appreciate it. You may need to: git fetch --all
git checkout more-tests
git reset --hard origin/more-tests since I modified previous commits. Thank you for all of your help! 😄 |
Thank you @blag for your work ! So I retried, indeed had to merge migrations 0009 and 0010.
Checked the And the
|
@chris-y-meyers I think I fixed that issue. Can you test again? The |
Hi @blag, Thanks for fixing the issue.
|
@chris-y-meyers I fixed that, and did a bunch more work, mostly involving tests. I think everything should be working now, and we now have a healthy amount of tests that all pass. Can you test everything once more for me? I'm almost positive this will be the last time. |
@blag can confirm that import process works right till end ! |
Finally! I'll merge it to master and push it to PyPI. Thank you for all your help! 😍 |
Closing - pushed to PyPI as version 0.5. Thank you all for your help! |
running the following command on command line produces the following error:
Seems to be missing an import on the cities management command in the current master branch.
The text was updated successfully, but these errors were encountered: