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

XLIFF Sync - New Build with Translations command #122

Merged
merged 14 commits into from
Aug 16, 2024

Conversation

FlorianNoeverGOB
Copy link
Contributor

These changes were being stated very useful for faster development by my Company

Build with Translations

This adds a new command xliffSync.buildWithTranslations that combines functionalities to:

  1. Delete current translation files
  2. Build App using the build command defined in new Setting Build Command To Execute
  3. Create new Target files for Languages defined in new Setting Default Languages
  4. Syncs Translations files to show if translations are missing

Therefore 2 new settings are created:

Default Languages

Xliff file generation now uses the languages defined in this Setting
Default Languages Setting
When the setting is empty (default) the user is asked to specifiy the languages (as normally)

Build Command To Execute

When using the new Command Build with Translations the command specified in this Setting will be used for building the App
Build Command To Execute Setting


There are multiple whitespace changes due to VSCode handling the formatting.

if there are any questions or anything else feel free to contact me :)

@rvanbekkum
Copy link
Owner

rvanbekkum commented Jul 25, 2024

Thank you. This indeed looks very useful! 😊

I will check the PR more thoroughly, but I do already have some small suggestions for changes before we merge this into the develop branch.

EDIT 2024-07-25 17:15 (CEST): I have left my initial comments. Could you please have a look at resolving those first?

@rvanbekkum rvanbekkum self-requested a review July 25, 2024 15:15
@rvanbekkum rvanbekkum added the awaiting changes Waiting for changes/response label Jul 25, 2024
@FlorianNoeverGOB
Copy link
Contributor Author

Thank you. This indeed looks very useful! 😊

I will check the PR more thoroughly, but I do already have some small suggestions for changes before we merge this into the develop branch.

EDIT 2024-07-25 17:15 (CEST): I have left my initial comments. Could you please have a look at resolving those first?

I can't seem to find any comments. But I don't use GitHub that much so perhaps I missed them. I looked in Files changed where I can make comments to files (lines) but there wasn't any comment. Where did you put them? 😅

README-NEW-FEATURES.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@rvanbekkum
Copy link
Owner

I can't seem to find any comments. But I don't use GitHub that much so perhaps I missed them. I looked in Files changed where I can make comments to files (lines) but there wasn't any comment. Where did you put them? 😅

Apparently I needed to click on a Submit Review button before the comments would be visible. 😅
(Shows that I am more used to Azure DevOps where the comments are posted directly 🙊)

You should be able to see them now.

@FlorianNoeverGOB
Copy link
Contributor Author

FlorianNoeverGOB commented Jul 26, 2024

Applied all comments. Take your time checking this, there is no hurry 😄 . I reacted with "👍" on every comment I "solved" if you agree with me you can "resolve" the comment for real (I would say I don't resolve myself cause maybe you wont agree with my "solving" so I just react)

README.md Outdated Show resolved Hide resolved
@rvanbekkum rvanbekkum added awaiting changes Waiting for changes/response enhancement New feature or request and removed awaiting changes Waiting for changes/response labels Jul 26, 2024
@rvanbekkum rvanbekkum removed the awaiting changes Waiting for changes/response label Jul 26, 2024
Copy link
Owner

@rvanbekkum rvanbekkum left a comment

Choose a reason for hiding this comment

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

Just took some more time to better review your PR contentwise.

I was wondering why the "Build with Translations" command is deleting existing translation files?
I would prefer it to reuse the existing translation files, so that you don't lose your existing translations. I don't see the need to remove them and I think it might work counterproductive.

Could you check and/or explain?

Copy link
Owner

Choose a reason for hiding this comment

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

Rename (remove the custom_).
Also for the other new files.

Copy link
Owner

Choose a reason for hiding this comment

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

Now that I think of it, these files are probably not referenced anymore now that the 'custom README' was removed.

Copy link
Owner

Choose a reason for hiding this comment

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

This one can be removed.

package.json Outdated
"type": "array",
"items": {
"type": "string",
"pattern": "^[a-z]{2}-[A-Z]{2}$"
Copy link
Owner

Choose a reason for hiding this comment

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

There are also some language tags that don't follow this pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a pattern that works for all? Or is it the best to just remove the pattern entirely?

Copy link
Owner

Choose a reason for hiding this comment

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

They are RFC 4646 or RFC 5646 language tags, but I think it might be better to just remove the pattern as I otherwise a RegEx that is a bit too complex. I think it's good enough if people can type in anything they want and think for themselves whether it is a valid tag. ;)

*
* @param {WorkspaceFolder} workspaceFolder The folder to restrict the search to.
*
* @returns A boolean that specifies wether translation files exist in the current workspace.
Copy link
Owner

Choose a reason for hiding this comment

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

wether -> whether

*
* @returns A boolean that specifies wether translation files exist in the current workspace.
*/
public static async checkXliffFilesExist(workspaceFolder?: WorkspaceFolder): Promise<boolean> {
Copy link
Owner

@rvanbekkum rvanbekkum Aug 2, 2024

Choose a reason for hiding this comment

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

Rename to xliffFilesExist

@@ -381,3 +444,13 @@ async function autoRunTranslationChecks(workspaceFolder?: WorkspaceFolder, targe

await runTranslationChecksForWorkspaceFolder(autoCheckMissingTranslations, autoCheckNeedWorkTranslations, targetUri, workspaceFolder);
}

function getCurrentWorkspaceFolder(): WorkspaceFolder | undefined {
Copy link
Owner

@rvanbekkum rvanbekkum Aug 2, 2024

Choose a reason for hiding this comment

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

Use method from WorkspaceHelper for selection of workspace folder(s) instead

@rvanbekkum rvanbekkum added the awaiting changes Waiting for changes/response label Aug 2, 2024
@FlorianNoeverGOB
Copy link
Contributor Author

Just took some more time to better review your PR contentwise.

I was wondering why the "Build with Translations" command is deleting existing translation files? I would prefer it to reuse the existing translation files, so that you don't lose your existing translations. I don't see the need to remove them and I think it might work counterproductive.

Could you check and/or explain?

So basically we don't use the Sync Feature because only our programmers create the Translations. So we don't want to have the "needs-work" (or how it was called) tags, because we always use the translations we created in the files. So if for example I add a new Label with a translation or change the translation of a label, that new translation will only get reviewed in the associated Pull Request by the Reviewer and then go directly into the Customers System. If I were to use the Sync command after I changed the Translation, I would only get the "needs-work" Tag added in the xliff files but not the new Translation (right? 😅). But as the new Translation is directly also the "final form" it should get added to the xliff files as normal which is why we normally delete the xliff files and call "create new target files" again.

I hope that was understandable 😅 if something was unclear please let me know 😄

@rvanbekkum
Copy link
Owner

So basically we don't use the Sync Feature because only our programmers create the Translations. So we don't want to have the "needs-work" (or how it was called) tags, because we always use the translations we created in the files. So if for example I add a new Label with a translation or change the translation of a label, that new translation will only get reviewed in the associated Pull Request by the Reviewer and then go directly into the Customers System. If I were to use the Sync command after I changed the Translation, I would only get the "needs-work" Tag added in the xliff files but not the new Translation (right? 😅). But as the new Translation is directly also the "final form" it should get added to the xliff files as normal which is why we normally delete the xliff files and call "create new target files" again.

I hope that was understandable 😅 if something was unclear please let me know 😄

Do you mean you use the parseFromDeveloperNote functionality?
If so, then that has some configuration options in place for when you would like to have it overwrite translations (xliffSync.parseFromDeveloperNoteOverwrite) and/or ignore source text changes (xliffSync.detectSourceTextChanges).
If you see any problems with using these settings, then please let me know as well.

Considering there are also companies that do not use the parseFromDeveloperNote functionality (our company included), I think it would be better to have this "Build with Translations" do a "build + sync" instead of a "build + delete + recreate" so that you do not lose the translations you had in the target translation .xlf files.

Is that something you could change?

@FlorianNoeverGOB
Copy link
Contributor Author

So basically we don't use the Sync Feature because only our programmers create the Translations. So we don't want to have the "needs-work" (or how it was called) tags, because we always use the translations we created in the files. So if for example I add a new Label with a translation or change the translation of a label, that new translation will only get reviewed in the associated Pull Request by the Reviewer and then go directly into the Customers System. If I were to use the Sync command after I changed the Translation, I would only get the "needs-work" Tag added in the xliff files but not the new Translation (right? 😅). But as the new Translation is directly also the "final form" it should get added to the xliff files as normal which is why we normally delete the xliff files and call "create new target files" again.
I hope that was understandable 😅 if something was unclear please let me know 😄

Do you mean you use the parseFromDeveloperNote functionality? If so, then that has some configuration options in place for when you would like to have it overwrite translations (xliffSync.parseFromDeveloperNoteOverwrite) and/or ignore source text changes (xliffSync.detectSourceTextChanges). If you see any problems with using these settings, then please let me know as well.

Considering there are also companies that do not use the parseFromDeveloperNote functionality (our company included), I think it would be better to have this "Build with Translations" do a "build + sync" instead of a "build + delete + recreate" so that you do not lose the translations you had in the target translation .xlf files.

Is that something you could change?

Yes we use parseFromDeveloperNote. Actually these are our settings:
image

I am unsure whether a Build + Sync would work. It should but I remember there was a case where it wouldn't work which is why we always do this for updating the translations in our company:
Build -> Delete Target Translation Files -> createNewTargetFiles

Im going to Test a bit whether I can find that case again where it wouldn't work. There is also the possibility of having both in 2 commands: Build with Translations (Sync) and Build with Translations (Recreate)

@FlorianNoeverGOB
Copy link
Contributor Author

So basically we don't use the Sync Feature because only our programmers create the Translations. So we don't want to have the "needs-work" (or how it was called) tags, because we always use the translations we created in the files. So if for example I add a new Label with a translation or change the translation of a label, that new translation will only get reviewed in the associated Pull Request by the Reviewer and then go directly into the Customers System. If I were to use the Sync command after I changed the Translation, I would only get the "needs-work" Tag added in the xliff files but not the new Translation (right? 😅). But as the new Translation is directly also the "final form" it should get added to the xliff files as normal which is why we normally delete the xliff files and call "create new target files" again.
I hope that was understandable 😅 if something was unclear please let me know 😄

Do you mean you use the parseFromDeveloperNote functionality? If so, then that has some configuration options in place for when you would like to have it overwrite translations (xliffSync.parseFromDeveloperNoteOverwrite) and/or ignore source text changes (xliffSync.detectSourceTextChanges). If you see any problems with using these settings, then please let me know as well.
Considering there are also companies that do not use the parseFromDeveloperNote functionality (our company included), I think it would be better to have this "Build with Translations" do a "build + sync" instead of a "build + delete + recreate" so that you do not lose the translations you had in the target translation .xlf files.
Is that something you could change?

Yes we use parseFromDeveloperNote. Actually these are our settings: image

I am unsure whether a Build + Sync would work. It should but I remember there was a case where it wouldn't work which is why we always do this for updating the translations in our company: Build -> Delete Target Translation Files -> createNewTargetFiles

Im going to Test a bit whether I can find that case again where it wouldn't work. There is also the possibility of having both in 2 commands: Build with Translations (Sync) and Build with Translations (Recreate)

Ok so lets say we have a Project with some custom Code and ofcourse Labels and Translations for them. The Customer now wants to change the tooltip of a field. Now I go and add whatever the request was as the new Tooltip and its translation.
I build the using al.package (ctrl+shift+b)

SyncTranslationUnits:
If I now execute the sync command. The field gets a new state needs-adaptation for the translation. Because the source text (the original english tooltip) changed. And now I don't have the new tooltip and its translation but still the old one.
If I would only change the translation of the tooltip, this would work fine. But If I also change the original source text we have this problem.

CreateNewTargetFiles:
If I instead delete the translation files and create them with the create command. The new tooltip is there with its translation. So now I can PR my changes into the main branch and make an update for the Customers System so he has the new tooltip

So basically I could either provide both variants with my build command like I said in my last message.
Or we would need an option for ignoring source text changes and just automatically translate them without getting the 'needs-adaptation' state.

@rvanbekkum
Copy link
Owner

Or we would need an option for ignoring source text changes and just automatically translate them without getting the 'needs-adaptation' state.

That already exists, i.e., xliffSync.detectSourceTextChanges. See my earlier message.
So if you use that setting then you should be good to go. 😉

@FlorianNoeverGOB
Copy link
Contributor Author

Or we would need an option for ignoring source text changes and just automatically translate them without getting the 'needs-adaptation' state.

That already exists, i.e., xliffSync.detectSourceTextChanges. See my earlier message. So if you use that setting then you should be good to go. 😉

Thanks totally forgot that xD.
It seems that there is a bug though? The original (en-US) still has needs-adaptation and the old translation. The other translation file (in my case de-DE) does get updated correctly though.

@rvanbekkum
Copy link
Owner

I am not sure what you mean. The base translation file (.g.xlf) is what the AL compiler generates and contains the source text in English (United States) en-US. (Strictly seen, therefore there is also not really a need for a separate .en-US.xlf file)
XLIFF Sync just synchronizes to all other (target) .xlf files that you may have in your project.

If you would like me to have a look maybe you can share a sample project/xliff files to reproduce the problem, together with the VS Code settings you are using. You could also file that as a GitHub issue in this repository.

@FlorianNoeverGOB
Copy link
Contributor Author

We always use en-US and (mostly) de-DE as our languages. So 3 files:
.d.xlf
en-US.xlf
de-DE.xlf

(en-US so you could in theory sent it someone ... i guess)
And the problem I mentioned only appeard on the en-US.xlf file. It seems to be just that. Use languages en-US and some other. change some translation and source text (while using detectSource: false and parseDeloperNoteOverwrite: true) and build + sync. en-US now still has needs-adaptation and old translation (old source text). But if you want I can create a sample project.

I will also make an Issue to this.

@FlorianNoeverGOB
Copy link
Contributor Author

Everything works now. I will modify the command to just build + sync now

@rvanbekkum
Copy link
Owner

Hi @FlorianNoeverGOB,

Thanks for the changes. I have made some last changes to the PR and pushed a commit. I have replaced the statements that invoked the synchronization with a call to an existing method for it and I made some updates to the CHANGELOG and README.
Could you please check these changes before I go over and merge them?

I have tested the functionality some further and it seems to work great! 😊

@rvanbekkum rvanbekkum removed the awaiting changes Waiting for changes/response label Aug 15, 2024
@FlorianNoeverGOB
Copy link
Contributor Author

Hi @FlorianNoeverGOB,

Thanks for the changes. I have made some last changes to the PR and pushed a commit. I have replaced the statements that invoked the synchronization with a call to an existing method for it and I made some updates to the CHANGELOG and README. Could you please check these changes before I go over and merge them?

I have tested the functionality some further and it seems to work great! 😊

Also tested it and seems to be working fine. So you can go ahead and merge 👍

@rvanbekkum rvanbekkum merged commit 6d7a2f4 into rvanbekkum:develop Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants