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

Remove map related code in scripts handling both geo data and translations #6241

Merged

Conversation

raksooo
Copy link
Member

@raksooo raksooo commented May 10, 2024

This PR removes the parts that handle map data and translations of relay locations from the extract-geo-data.py and integrate-into-app.py scripts. It also renames those scripts. The remaining parts are for fetching the relay list and adding all locations to the translation template (relay-locations.pot).


This change is Reviewable

@raksooo raksooo requested a review from dlon May 10, 2024 12:36
Copy link

linear bot commented May 10, 2024

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @raksooo)


gui/scripts/fetch-relay-locations.py line 52 at r1 (raw file):

    country_code, city_code = location_key.split("-")

    if country_name is None:

Can this ever be None? Doesn't look like it. It can be an empty string, though.


gui/scripts/fetch-relay-locations.py line 56 at r1 (raw file):

      continue

    if city_name is None:

Can this ever be None? Doesn't look like it. It can be an empty string, though.


gui/scripts/fetch-relay-locations.py line 65 at r1 (raw file):

    country = countries[country_code]
    cities = country["cities"]
    if location_key != "se-bet":

Is this still a thing? I can't recall seeing any "beta" relays.

@raksooo raksooo force-pushed the update-scripts-that-generate-messagespot-and-relay-des-837 branch from 2135ecc to 9eabe74 Compare May 15, 2024 16:49
Copy link
Member Author

@raksooo raksooo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @dlon)


gui/scripts/fetch-relay-locations.py line 52 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Can this ever be None? Doesn't look like it. It can be an empty string, though.

I'm not very experienced with Python, but from my understanding this would be None if location doesn't have a country, which would only happen if the the relay list doesn't look as expected, and IMO it makes sense to account for that.

This code isn't new, it's been copied from the old scripts that did much more than what this one is doing.


gui/scripts/fetch-relay-locations.py line 65 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Is this still a thing? I can't recall seeing any "beta" relays.

I've never read this script thoroughly enough to see this 😆 Removed.

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @raksooo)


gui/scripts/fetch-relay-locations.py line 42 at r1 (raw file):

  for location_key in locations:
    location = locations.get(location_key)

This can be rewritten as

for location_key, location in location.items():

Code quote:

  for location_key in locations:
    location = locations.get(location_key)

gui/scripts/fetch-relay-locations.py line 52 at r1 (raw file):

Previously, raksooo (Oskar Nyberg) wrote…

I'm not very experienced with Python, but from my understanding this would be None if location doesn't have a country, which would only happen if the the relay list doesn't look as expected, and IMO it makes sense to account for that.

This code isn't new, it's been copied from the old scripts that did much more than what this one is doing.

My bad, I must have gotten country_name and country_code mixed up. It would be less misleading to assign the variables immediately before checking if they're None:

country_name = location.get('country')
if not country_name:
    ...
city_name = location.get('city')
if not city_name:
    ...

gui/scripts/fetch-relay-locations.py line 125 at r2 (raw file):

  extract_relay_translations()

main()

Not too important, but typically you only call main() if this is the top-level module:

if __name__ == '__main__':
    main()

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @raksooo)

Copy link
Member Author

@raksooo raksooo left a comment

Choose a reason for hiding this comment

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

Leaving code as is since this PR hasn't changed that code.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@raksooo raksooo merged commit 970fb07 into main May 16, 2024
26 checks passed
@raksooo raksooo deleted the update-scripts-that-generate-messagespot-and-relay-des-837 branch May 16, 2024 08:46
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