CI: obs-crowdin-sync Python Rewrite

[ghost]

Description

Rewrites the obs-crowdin-sync repository in Python and adds the action to this repository.

Changes that come along with it:

• Translates the UI/cmake/linux/com.obsproject.Studio.metainfo.xml.in file.
• Creates a pull request instead of pushing directly: https://github.com/Vainock/obs-studio/pull/4
• Changes the name of English to English (USA) for en-US.
• Lowers the requirements for new languages to be added to the UI/data/locale.ini.
• Host the Language string in a separate Language Name file on Crowdin, remove it here.

Makes the organization/repository secret CROWDIN_SYNC_GITHUB_PAT obsolete.

Motivation and Context

The obs-crowdin-sync repository was too much for what it solves.

This solution is better for integration testing, easier to extend, easier for error analysis in the log because you don't have a huge bundled script and much cleaner to look at.

Pushing directly to the repo is bad, creating a PR is safer and transparent.

How Has This Been Tested?

Locally and https://github.com/Vainock/obs-studio/pull/4. The update-crowdin-locales action was tested in a obs-studio and obs-browser fork.

Types of changes

• Tweak (non-breaking change to improve existing functionality)

Checklist:

• My code has been run through clang-format.
• I have read the contributing document.
• My code is not on the master branch.
• The code has been tested.
• All commit messages are properly formatted and commits squashed where appropriate.
• I have included updates to all appropriate documentation.

[tytan652]

Blocked by #10658, once merged please update your code to only translate UI/cmake/linux/com.obsproject.Studio.metainfo.xml.in and not UI/xdg-data/com.obsproject.Studio.metainfo.xml.in.

The latter is in the deprecated CMake path, the former is the new one.

[ghost]

@tytan652 Should it also update UI/cmake/linux/com.obsproject.Studio.desktop instead of UI/xdg-data/com.obsproject.Studio.desktop?

[tytan652]

Yes

I think the change about going away from xdg-data folder has completely gone unseen. We found out about it recently through metainfo regression in CI.

[ghost]

Ready for review.

[PatTheMav]

Is list_directories assured to always return a dictionary with an non-empty array with key data, which itself will contain objects that always contain an object at key data which has a key id?

If the answer is "no" or "maybe" use a chain of get calls instead:

directories = crowdin_api.source_files.list_directories(filter=name).get("data", [])

if len(directories) > 0:
  id = directories[0].get("data", {}).get("id", None)
  if id is not None:
    return id
  else:
    raise Exception("No id field on directory data")
else:
  raise Exception("No crowdin source file directories found")

[WizardCM]

I have checked this, and the Crowdin documentation explicitly states that ["data"][0]["data"] are all guaranteed ("required") in the response, if the response is 200.

[PatTheMav]

Wouldn't be the first time a response header states 200 and the result is an empty string, or it's truncated, or it's malformed, or it's actually a piece of HTML.

There is no good reason not to write code that is guaranteed to succeed instead of code that just doesn't seem to fail.

[WizardCM]

True enough. What I also just realised is that function is missing a return.

[PatTheMav]

See above re. avoidance of ValueErrors.

[ghost]

When I try...except this theses object accesses, what should I do in the except block? I caught the error, but should I raise an exception again? Just log it?

My only idea would be to try...except the upload() function in the main function (https://github.com/Vainock/obs-studio/blob/ba977d75bd6f8e1546f0d2824d6714110073af31/.github/scripts/utils.py/update-crowdin-locales.py#L101) for all KeyErrors and ValueErrors. This would cover all instances where these exceptions can occur in that file. My problem is that I can only return the error code from the main function. Do you have an idea for how to solve that?

[PatTheMav]

You don't have to catch exceptions right where they happen but where you can handle them, so it'd be fine to not handle them on lower levels and catch them on the top-most level and log the error there and return from the main function with an error code.

The exception to that would be if you want to be able to more specifically identify the error to have more specific error messages (so you want to differentiate which interaction resulted in a ValueError).

This would also enable you to raise errors with error messages in the inner functions instead of having to log error messages, because you're catching them in main and print the error there.

You usually raise the exceptions in the lowest levels, ignore them in the middle layers which just pipe data back and forth and catch them at the levels the user interacts with (because it's where you'd need to compose error messages, display them to users, ask them how to handle the error, etc.).

It's considered good practice to document this though:

    def someFunction(someArgument: someType) -> Int
       """This does thing

       Returns:
           Int: The number of thing.

       Raises:
            MyException - In case something happen
       """

[PatTheMav]

Multi-dimensional direct access will lead to ValueError if key doesn't return dictionary with the expected key. Use chained get calls to ensure the access does not fail and use a fallback like None to check for errors.

[ghost]

If the dictionary access fails, there is no reason to recover from there on. If there is no exportPattern in exportOptions, then the configuration on Crowdin needs to be updated. The code depends a lot on the configuration on Crowdin and assumes a specific structure. I support making the code flexible to the API results, but have trouble understanding what makes encapsulating all possible situations with error handling because of possible misconfiguration on Crowdin or broken responses beneficial.
Adding optional debug logging for all responses makes in my head more sense.

[PatTheMav]

Every time you access an API or process data you're not in control of, you have to ensure to either safely evaluate the state of the data before accessing it to stay in control of program execution (otherwise you openly rely on undefined state and you can make no assumptions of the states your code can end up in).

If any issue while accessing that data is considered a possible error, at the very least wrap the access in a try: [...] except (ValueError, KeyError) as e: block because the code can potentially break on both of these errors because of an invalid access violation. You can then handle the error by logging an appropriate error function, clean up, and exit the program with an error code under your control.

[PatTheMav]

See the comments for update-crowdin-locales.py regarding globals, lack of a main function and command-line arguments, as well as error-prone deep access of nested dictionary keys without proper error handling (at the very least a try.. except block catching a ValueError would be necessary).

[PatTheMav]

Don't use upper case naming for anything but actual constants.

If these are actually constants, they should either be declared global in the file scope or encapsulated (the latter is more elegant because it doesn't pollute the file scope, but not strictly necessary):

class OBSCrowdinConstants:
  FILE_PATH = ""
[..]

def write_locale_file(build: CrowdinBuild) -> None:
  [..]
  with open(OBSCrowdinConstants.FILE_PATH, encoding="utf-8") as file:
    [...]