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

Refactor convert_jsons_to_xcstrings.py for Improved File Handling and Error Management #62

Merged

Conversation

OmarAI2003
Copy link
Contributor

This PR introduces several improvements discussed here to the convert_jsons_to_xcstrings.py script used for converting Scribe-i18n's Localizable.xcstrings file to localization JSON files.

Changes Made

  1. Folder Existence Check:

    • Added a check to ensure that the "jsons" folder exists before attempting to read files. If the folder does not exist, the script will print an error message and exit gracefully.
  2. Safe File Handling:

    • Replaced direct file reading with context managers (with open(...)) to ensure files are properly closed after being read or written, thus improving resource management.
  3. Loading Language Files:

    • Pre-loaded all language JSON files into a dictionary at once, reducing the need for repeated file reads during the translation process. This improves performance and clarity.
  4. Improved Key Handling:

    • Used dict.get() for retrieving translations to simplify the logic and improve readability.

Notes

  • Future work could include further modularization and unit testing to enhance the robustness of the codebase.

- Refactored the code to load all language JSON files into memory once, reducing redundant file access.
- Improved translation lookup efficiency by using a dictionary for language data, resulting in faster processing of keys.
- Enhanced overall script performance, especially beneficial for larger datasets
- Added a check to ensure the 'jsons' folder exists, exiting with an error message if it is missing.
- Implemented a try-except block for loading 'en-US.json' to catch FileNotFoundError.
- Added a user-friendly error message to indicate if the base language file is missing.
- Changed variable name from 'file' to 'base_language_data' for better readability and to clearly indicate its purpose.
Copy link

github-actions bot commented Oct 7, 2024

Thank you for the pull request!

The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and i18n rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you!

Maintainer checklist

  • The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution
    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local activist repo

@andrewtavis andrewtavis requested a review from Jag-Marcel October 7, 2024 01:08
@andrewtavis
Copy link
Member

Could you check this out for a first check as with the last one, @Jag-Marcel? :)

@@ -10,35 +10,53 @@
import os

directory = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
json_dir_list = os.listdir(os.path.join(directory, "jsons"))
# Define the path to the "jsons" folder
Copy link
Member

Choose a reason for hiding this comment

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

Quick thing, @OmarAI2003: For Pythonic comments if it's its own line then there should be punctuation, so this should have a period.


translation = lang_json[key] if key in lang_json else ""
for key in base_language_data: # Iterate over each key in the base language file
Copy link
Member

Choose a reason for hiding this comment

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

These commends though don't need punctuation as you have them, but then as they're inline the first letter shouldn't be capitalized. The first word of a comment is only capitalized when it's its own line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These commends though don't need punctuation as you have them, but then as they're inline the first letter shouldn't be capitalized. The first word of a comment is only capitalized when it's its own line.

I’ve made the edits to the comments as suggested. Thank you for pointing out the details; I’m learning a lot from your feedback!

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

Very minor changes here @OmarAI2003 :) One thing to note is that we should always do inline comments two spaces after the code that they follow as this is black/ruff code standard. We don't have formatting in this repo as we want to keep it as small as we can and it's just four Python scripts. No stress :)

Thanks for your continued support for these scripts!

@andrewtavis andrewtavis merged commit 7dbeaa1 into scribe-org:main Oct 7, 2024
@OmarAI2003 OmarAI2003 deleted the improve-convert-jsons-to-xcstrings branch October 8, 2024 05:12
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