Skip to content

Bug 1967617 - Replace individual Geoname::admin{level}_code properties with one admin_division_codes map #6758

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

Open
wants to merge 2 commits into
base: adw/geonames-l10n-bug-1958992
Choose a base branch
from

Conversation

0c0w3
Copy link
Contributor

@0c0w3 0c0w3 commented May 20, 2025

This builds on #6745 and replaces the individual Geoname::admin{level}_code properties with a single admin_division_codes map. A few reasons:

  • I'm working on another patch that adds a way for consumers to fetch localized/alternate names for geonames, including alternates for a geoname's admin divisions. I'm introducing another struct for that, and I don't want to repeat admin{level}_name for each level in that struct.
  • I like the idea of not baking the max number of levels into the public API.
  • Not a big deal, but a map is similar to how AccuWeather's location API handles admin divisions, although it uses an array of objects instead of one object.

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

…ies with one `admin_division_codes` map

This replaces the individual `Geoname::admin{level}_code` properties with a
single `admin_division_codes` map. A few reasons:

* I'm working on another patch that adds a way for consumers to fetch
  localized/alternate names for geonames, including alternates for a geoname's
  admin divisions. I'm introducing another struct for that, and I don't want to
  repeat `admin{level}_name` for each level in that struct.
* I like the idea of not baking the max number of levels into the public API.
* Not a big deal, but a map is similar to how AccuWeather's location API handles
  admin divisions, although it uses an array of objects instead of one object.
@0c0w3 0c0w3 requested review from ncloudioj and tiftran May 20, 2025 22:35
@0c0w3
Copy link
Contributor Author

0c0w3 commented May 20, 2025

This doesn't work with UniFFI because the HashMap is converted to a JS object, and the keys of JS objects are strings. So when UniFFI checks that the keys are ints, it throws an error.

Not sure what to do. Either use an array of objects like AccuWeather does -- [{ level: 1, code: "ENG" }] -- or make a UniFFI patch that converts HashMap to JS Map instead of plain objects, or something.

@0c0w3
Copy link
Contributor Author

0c0w3 commented May 20, 2025

I'm going to submit a UniFFI patch ASAP. This PR aside, using a plain JS object for a map is a straight up bug. So let's go forward with this PR assuming that will be fixed, but let me know if you think using a map for the admin divisions is a bad idea aside from that.

Copy link
Member

@ncloudioj ncloudioj left a comment

Choose a reason for hiding this comment

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

:shipit:

@0c0w3
Copy link
Contributor Author

0c0w3 commented May 21, 2025

This doesn't work with UniFFI because the HashMap is converted to a JS object, and the keys of JS objects are strings. So when UniFFI checks that the keys are ints, it throws an error.

There was a bug already on file: Bug 1809459 - Use Map instances for UniFFI maps to allow non-string keys.

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