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

Change city_lookup to load all configured cities #68

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

duduribeiro
Copy link

@duduribeiro duduribeiro commented Nov 24, 2023

Closes #67
There is a problem with city_lookup. It only loads the cities for the first statue you called cities on. This is because cities memoizes all the cities for the country, but city_lookup only loads the cities for the state, and doesn't get called again for the other states.

Ex:

having the following db/cities-lookup.yml:

BR:
  GO:
    "Mutunópolis": "Mutunópolis"

If I load first cities for other state, it does not include this missing city:

bin/rails c
CS.cities(:sp, :br) # loading another state first
CS.cities(:go, :br).include?("Mutunópolis")

=> false

returned false but it should be true.

If I load this state first, it works correctly

bin/rails c
CS.cities(:go, :br).include?("Mutunópolis")

=> true

This commit changes city_lookup to load all the cities for the country.

Closes thecodecrate#67
There is a problem with city_lookup. It only loads the cities for the
first statue you called `cities` on. This is because `cities` memoizes
all the cities for the country, but `city_lookup` only loads the cities
for the state, and doesn't get called again for the other states.

Ex:

having the following `db/cities-lookup.yml`:

```yaml
BR:
  GO:
    "Mutunópolis": "Mutunópolis"
```

If I load first cities for other state, it does not include this missing
city:

```shell
bin/rails c
CS.cities(:sp, :br) # loading another state first
CS.cities(:go, :br).include?("Mutunópolis")

```

returned false but it should be true.

If I load this state first, it works correctly

```shell
bin/rails c
CS.cities(:go, :br).include?("Mutunópolis")

```

This commit changes `city_lookup` to load all the cities for the country.
@viviancan
Copy link

@duduribeiro Hello 👋 Any updates on this^? We are facing a similar issue when trying to add cities using the cities-lookup file. I have successfully renamed a country and state using the country and state lookup files but have not been able to add a city. Thx!

@duduribeiro
Copy link
Author

hey @viviancan 👋

I'm not a maintainer of the repo, so I can't merge.

What you can do is use this commit (like I'm doing in my app).

You can add this on your Gemfile

gem "city-state", github: "duduribeiro/city-state", branch: "fix-lookup-cities"

and check if it works on your case. It worked for me

@viviancan
Copy link

hey @viviancan 👋

I'm not a maintainer of the repo, so I can't merge.

What you can do is use this commit (like I'm doing in my app).

You can add this on your Gemfile

gem "city-state", github: "duduribeiro/city-state", branch: "fix-lookup-cities"

and check if it works on your case. It worked for me

@duduribeiro Thank you for the tip 😄 Hopefully there will be some feedback on this issue soon!

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.

Adding a missing city is not working properly
2 participants