Skip to content

Conversation

NMAC427
Copy link
Contributor

@NMAC427 NMAC427 commented Oct 19, 2020

Fixes #151

@Jeehut
Copy link
Member

Jeehut commented Nov 15, 2020

@NMAC427 I just saw this in my GitHub inbox, sorry for not checking it earlier. Didn't know you actually provided an implementation here. Code looks good on first glance, but could you also please add a test which will ensure this continues to work even when I decide to do some refactoring later? Also, could you please update the README accordingly? Thanks! 👍

@Jeehut
Copy link
Member

Jeehut commented Feb 21, 2021

I just had another look and I'm happy to merge this, but I'd really like to have at least one simple test and a simple addition to the README that explains how this can be turned on or off. If possible, I think we should make this opt-in with the current version so users with an already setup configuration don't get confused by a new bahavior.

@NMAC427 @patricks @RSickenberg Would any of you be up to completing this?

@somenkovnikita
Copy link

I tried this PR, it works, but noticed that it doesn't pick up my translations from Base. Just creates a list of keys with an empty target. I'm not sure if this is convenient. I would say that it is more correct to copy the translation itself by key .+FormatString.

I.e. for example, for input:

<array>
    <dict>
        <key>INIntentParameterPromptDialogCustom</key>
        <true/>
        <key>INIntentParameterPromptDialogFormatString</key>
        <string>Hostname ${hostName}</string>
        <key>INIntentParameterPromptDialogFormatStringID</key>
        <string>n31mnR</string>
        <key>INIntentParameterPromptDialogType</key>
        <string>Primary</string>
    </dict>
</array>

In <sourceLocale-from-config>.lproj/Localization.string:

"n31mnR" = "Hostname ${hostName}";

Instead of:

"n31mnR" = "";

What do you think @NMAC427 @Jeehut?

@Jeehut
Copy link
Member

Jeehut commented Feb 21, 2021

@somenkovnikita I honestly have near to no knowledge about intent files and their translation format, so I'm the wrong person to ask. But I'd be happy to review & merge any changes to BartyCrouch as long as they are related to translations (this PR certainly is), have some tests and will not change the behavior for anyone who already has a BartyCrouch configuraton setup (as this could lead to confusion and would require a breaking change update).

Feel free to fork @NMAC427's PR and make adjustments as you think they are right. I'm dependent on the Community on this topic as I don't have the time to learn and improve stuff related to this topic myself anytime soon.

@RSickenberg
Copy link

I can surely test for some intent definitions files since I use them for https://teslacompanion.app/.

Just tell me what can I do to help! @Jeehut @NMAC427 @somenkovnikita

@patricks
Copy link
Contributor

I used this version now for a few translations, it worked without any problems. But it would be nice, if it also could add a comment with the base translation. Like the storyboard translation does.

@Jeehut
Copy link
Member

Jeehut commented Apr 5, 2021

I just had another look and I'm happy to merge this, but I'd really like to have at least one simple test and a simple addition to the README that explains how this can be turned on or off. If possible, I think we should make this opt-in with the current version so users with an already setup configuration don't get confused by a new bahavior.

@NMAC427 @patricks @RSickenberg Would any of you be up to completing this?

@RSickenberg @patricks I already mentioned exactly what is needed to get this merged, let me put it in a list form:

  1. One simple unit test (I don't care which part you test, do what's simplest)
  2. Document how this can be set up & used in the README.md file
  3. Make sure this does not change behavior for existing users by making it opt-in (rather then on by default)

Once these are provided, I'll merge right away and make a new release. Thank you for your help.

@Jeehut Jeehut force-pushed the main branch 2 times, most recently from 09bb412 to a436268 Compare October 10, 2021 09:36
@FlineDevPublic
Copy link
Collaborator

FlineDevPublic commented Jan 14, 2022

@patricks @RSickenberg @somenkovnikita @NMAC427
I've tried this out myself with some basic setup (note that I have never translated any SiriKit Intent Definition files before, so I might have misunderstood things), but this code didn't work for me. But as I'm currently trying to close all open issues for BartyCrouch, I'd like to get this merged.

So if you can help me understand why it didn't work for me or maybe even help fixing the issue, I would be very happy. I've rebased this PR and created a subsequent one in #241. Gonna close this, please let's continue the discussion in #241.

Here's the test intent structure where this code didn't work for me, maybe it's related to the Base issue mentioned by @somenkovnikita above:
TestFiles.zip


This comment was written during my regular Open Source live stream on Twitch. Follow me there to support my work!

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.

Siri shortcut translation files are not handled
6 participants