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

[DISCO-3294] chore: update more cities for weather #813

Merged
merged 2 commits into from
Mar 5, 2025
Merged

Conversation

tiftran
Copy link
Contributor

@tiftran tiftran commented Mar 4, 2025

References

JIRA: DISCO-3294

Description

Because we're cycling through city names, normalized city name, city name without locality suffix times # of regions for that city, we're making a lot of requests we don't need to. I've split REGION_MAPPING_EXCLUSIONS into two sets instead and added the countries that we know. In addition added the cities in those countries that require the city to be None.

PR Review Checklist

Put an x in the boxes that apply

  • This PR conforms to the Contribution Guidelines
  • The PR title starts with the JIRA issue reference, format example [DISCO-####], and has the same title (if applicable)
  • [load test: (abort|skip|warn)] keywords are applied to the last commit message (if applicable)
  • Documentation has been updated (if applicable)
  • Functional and performance test coverage has been expanded and maintained (if applicable)

@tiftran tiftran requested a review from a team March 4, 2025 01:08
@@ -32,7 +32,7 @@
"77",
),
(
Location(country="AR", regions=["B", "5"], city="La Plata"),
Location(country="AA", regions=["B", "5"], city="La Plata"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to a fake country code, since as we add more countries to our lists, this test will fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a comment for future reference.

@@ -89,6 +96,11 @@
("US", "UT", "Hill Air Force Base"): 0,
}

KNOWN_SPECIFIC_REGION_COUNTRIES: frozenset = frozenset(
Copy link
Contributor

Choose a reason for hiding this comment

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

We're getting constants defined here, let's add some descriptions to them for better readability.

@@ -32,7 +32,7 @@
"77",
),
(
Location(country="AR", regions=["B", "5"], city="La Plata"),
Location(country="AA", regions=["B", "5"], city="La Plata"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a comment for future reference.

@tiftran tiftran force-pushed the update-pathfinder branch from 40cff62 to 9e70509 Compare March 5, 2025 18:46
@tiftran tiftran added this pull request to the merge queue Mar 5, 2025
Merged via the queue into main with commit 90fb6e7 Mar 5, 2025
7 checks passed
@tiftran tiftran deleted the update-pathfinder branch March 5, 2025 19:02
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.

2 participants