Refactor convert_xcstrings_to_jsons.py for Improved File Handling and Error Management #58
Conversation
- Updated file reading to use a context manager for better resource management. - Added error handling for FileNotFoundError when accessing Localizable.xcstrings.
- Implement try-except block for JSONDecodeError to handle invalid JSON format.
- Refined language handling. - Added directory existence check and creation to prevent errors.
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
|
|
@Jag-Marcel, could I ask that you take a first look here? Thanks so much for the PR, @OmarAI2003! |
There was a problem hiding this comment.
Almost everything here looks really good to me! As for the issues with the file paths, @OmarAI2003 do you want to change them in your own PRs for #57 too?
| except FileNotFoundError: | ||
| print("Error: Localizable.xcstrings file not found.") | ||
| exit(1) | ||
|
|
||
| dir_list = os.listdir(directory) |
There was a problem hiding this comment.
I think I missed the fact we restructured the json files into their own directory, it should be os.path.join(directory, "jsons") with our current structure? @andrewtavis this could also be an issue with all the other scripts.
There was a problem hiding this comment.
I think I missed the fact we restructured the json files into their own directory, it should be
os.path.join(directory, "jsons")with our current structure? @andrewtavis this could also be an issue with all the other scripts.
I think os.path.join(directory, "Localizable.xcstrings" should remain as is or the file Localizable.xcstrings to be moved to the folder jsons as the file Localizable.xcstrings is in the root directory Scribe-i18n
should I move Localizable.xcstrings to the jsons folder since It's a JSON-like format?
There was a problem hiding this comment.
No let's keep it where it is, as that would complicate Weblate that's reading in the jsons directory :)
| json.dump(data, dest, indent=2, ensure_ascii=False) | ||
| dest.write("\n") | ||
| # Write to the destination JSON file using a context manager | ||
| with open(f"{directory}/{lang}.json", "w") as dest: |
There was a problem hiding this comment.
Same thing as above goes here, I think it should be f"{directory}/jsons/{lang}.json".
| dir_list = os.listdir(directory) | ||
| languages = [file.replace(".json", "") for file in dir_list if file.endswith(".json")] | ||
|
|
||
| # Ensure the destination directory exists | ||
| if not os.path.exists(directory): |
There was a problem hiding this comment.
This check should probably go before dir_list is created on line 23, since it already uses the directory. I don't think we need to check directory itself, since that's supposed to be the root Scribe-i18n directory anyway. It could be worth it if there's a variable for the jsons directory and you do the same check on that instead.
… to 'jsons' directory
This commit restores the Localizable.xcstrings file to its previous directory as requested. Keeping the file in its original location simplifies integration with Weblate, which reads from the jsons directory.
There was a problem hiding this comment.
All's really great here, @OmarAI2003! I realized when I tried to run it that we were writing an en.json file, so I just switched en back to en-US for now. We can see how localization progresses in the future for us maybe having different dialects 😊
Would be great if you sent along similar changes to the other scripts! Very very welcome on our end :)
Thank you so much for your work and for adding the Hacktoberfest labels! I really appreciate it. 😊 I'll be happy to make similar changes to the other scripts as well. Is there any particular script you'd like me to start with? |
|
Let's finish the iOS ones and then move on to the Android ones, @OmarAI2003? :) |
This PR introduces several improvements discussed here to the
convert_xcstrings_to_jsons.pyscript used for converting Scribe-i18n'sLocalizable.xcstringsfile to localization JSON files.Changes Made:
File Handling Improvements:
Localizable.xcstringsfile and writing the output JSON files. This ensures that files are properly closed after use.Error Handling:
FileNotFoundErrorto handle cases where theLocalizable.xcstringsfile is missing.Directory Existence Check:
These improvements improve the file's reliability and maintainability, facilitating smoother execution and easier debugging. I welcome any feedback for further improvements.
Related issue:
#57