search

crowdin / github-action

A GitHub action to manage and synchronize localization resources with your Crowdin project
https://crowdin.com
MIT License
164 stars 56 forks source link

Not setting configured labels on PRs #251

Open torokati44 opened 1 month ago

torokati44 commented 1 month ago

These lines should probably use a /pulls/ URL, rather than /issues/...

https://github.com/crowdin/github-action/blob/c953b17499daa6be3e5afbf7a63616fb02d8b18d/entrypoint.sh#L161 https://github.com/crowdin/github-action/blob/c953b17499daa6be3e5afbf7a63616fb02d8b18d/entrypoint.sh#L178

See: https://github.com/ruffle-rs/ruffle/actions/runs/11021911424/job/30610062421#step:4:11 https://github.com/ruffle-rs/ruffle/actions/runs/11021911424/job/30610062421#step:4:576

Docs: https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#update-a-pull-request

andrii-bodnar commented 1 month ago

@torokati44 thanks for reporting this! It's strange, I wonder how it worked before.

I just made a change in the fix-pulls branch (812dd3c)

Could you please try it by referencing the branch instead of the version?

uses: crowdin/github-action@fix-pulls
torokati44 commented 1 month ago

Maybe it never worked...? 😐🫥 Thank you for the quick fix! I think actually testing it on our end would have a bit too much overhead, especially since we're in the process of moving away from this action anyway... 😶 ( https://github.com/ruffle-rs/ruffle/pull/18028 )

andrii-bodnar commented 1 month ago

since we're in the process of moving away from this action anyway

Could you please provide more details on this?

If you need to make some changes to the translation files, you are probably looking for post-export processing on the Crowdin side instead of doing it after the translations download.

torokati44 commented 1 month ago

Would you mind summarizing, @kjarosh?

kjarosh commented 1 month ago

We want to localize the following files:

https://github.com/ruffle-rs/ruffle/blob/b59ee2f91bf0c98c3fb6e200f8878c925bc5c84e/desktop/packages/linux/rs.ruffle.Ruffle.metainfo.xml

https://github.com/ruffle-rs/ruffle/blob/b59ee2f91bf0c98c3fb6e200f8878c925bc5c84e/desktop/packages/linux/rs.ruffle.Ruffle.desktop

Crowdin does not support them, so the plan is to:

  1. convert them to .pot and feed it to Crowdin on upload,
  2. download .pos and merge them into the final file on download.

Conversion tools include xgettext, msgfmt, and itstool.

andrii-bodnar commented 1 month ago

@kjarosh thanks for the details!

I just did a quick test and it seems that the provided XML file imports well without any modifications or convertations. You can additionally specify XPath translatable_elements to better configure what is translatable (Configuration File).

The *.desktop file looks very similar to ini. If you change the extension, it imports very well as an ini file format.

So, you can add an extra step to your workflow where you rename all the *.desktop files to *.ini, then do the sync via Crowdin action (with PR creation disabled). After that, rename ini files back and create a pull request. I hope this should work.

torokati44 commented 1 month ago

I believe the problem is that both the metainfo and desktop files have specific semantics of how they are supposed to store the localized texts (all in a single file) - it's not as simple as with other files, where it's just multiple instances of the same basic file (one per language). Hence these file formats (semantically speaking, not just about how they "look") would need tailor-made support from Crowdin. But correct me if I'm wrong.

andrii-bodnar commented 1 month ago

Do you mean that these files are multilingual?

kjarosh commented 1 month ago

@torokati44 is right, Crowdin indeed can translate those files, but we'll get an instance for each locale, which is not how metainfo and desktop files support localization.

The desktop file uses [<locale>] suffix for keys (for translatable elements), and metainfo uses inline xml:lang attributes (for translatable elements).

If we have to merge those files anyway, it's just easier to convert them to .pot/.po using standard methods.

Do you mean that these files are multilingual?

That's right, you can consult the documentation here:

andrii-bodnar commented 1 month ago

Thanks for the details! This sounds very reasonable to me now.

Earlier we had a similar discussion to allow running some pre-commit scripts, but there is no obvious solution on how to do it right (https://github.com/crowdin/github-action/discussions/181)

You can still synchronize the source files and translations with this action, but disable the automatic commit and PR creation.

kjarosh commented 1 month ago

You can still synchronize the source files and translations with this action, but disable the automatic commit and PR creation.

Yes, that's the plan. I think @torokati44 meant that we're moving away from this action regarding committing & creating PRs.

torokati44 commented 1 month ago

Indeed, pardon my imprecision.

torokati44 commented 1 month ago

Actually, it's entirely possible that the /issues/ URL does get redirected to the correct one, based on the number (if it actually belongs to a PR); and it doesn't work for us because the GITHUB_TOKEN we supply has no permission to manage labels... But in that case an error message about this would have been nice...