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

feat(translationtool): Allow vue-translator comments #268

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jotoeri
Copy link
Member

@jotoeri jotoeri commented Feb 28, 2021

Hi all,

i just gave it a go to extract translator-comments from vue-files. I fell over that in nextcloud/documentation#5422
I'm neither the great php pro, and not at all for Regex. The attempt is probably not the ideal one and it needs a bit more looping than before. But it seems to work. 🙈
So comments and review-suggestions are welcome! 😊

Three things from my side:

  • The regex check for TRANSLATORS is executed twice - Once for HTML and once for JS. This is due to the fact, that i didn't find a Regex to match either the HTML-EndTag --> or just nothing for JS-Comments. Does one of you have an idea here?
    EDIT: Just had an idea and pushed a fixup: Now checking for <!--|//|/* TRANSLATORS and then separately removing possible end-tags -->|*/
  • One issue i realised, but it is more or less independent and already existed before: Double-Quote matches most probably use double-quotes as they want to use a single quote within their strings. However, they are parsed to be single-quotes here, which does not work anymore. (I saw an issue on some app, but i don't find it anymore 😞) Any idea for this? Or probably do it as separate topic? 🤔
  • Can we test this somehow properly before merging / going live? I was only able to test my new code out of scope, but therefore changing some small code parts to make it run.

Greets! :)
Jonas

cc @rakekniven 😉

Fake-File Before:
grafik

Fake -File After:
grafik

while the first comment is out of HTML and the second one out of JS:
grafik
grafik

@rakekniven
Copy link
Member

@jotoeri Wow, thank you! Hope that reviews will be approved and it will hit ci soon ;-)

@jotoeri
Copy link
Member Author

jotoeri commented Mar 19, 2021

Any comments @juliushaertl @MorrisJobke @ChristophWurst ? 🤔

@@ -320,31 +320,56 @@ private function createFakeFileForVueFiles() {
$fakeFileContent = '';

foreach ($this->findTranslatableFiles(['.vue']) as $vueFile) {
$vueSource = file_get_contents($vueFile);
$vueSource = file($vueFile);
Copy link
Member

Choose a reason for hiding this comment

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

if you read line per line, do multi line t/n calls still match? I doubt it

try parsing code that says

const text = t(
  'test',
  'my text'
);

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, you're right yes.

So - the only idea i have then is a quite complex regex, that catches the full block. Just quickly tested this one and it seems to work on my testfile, also with the multiline translations.

Would you be ok with using such a regex? Or do you think it's too complex & error-prone? 🤔

$translatorsRegex = "(\W(\/\/|\/\*|<!--)\s(TRANSLATORS)\s(.+))*"; // Indicating an optional TRANSLATORS-comment
$inbetweenRegex = "(\s*.*\s*)"; // Text between comment and translation-function

preg_match_all("/". $translatorsRegex . $inbetweenRegex . "\Wt\s*\(\s*'([\w]+)',\s*'(.+)'" . "/", $vueSource, $singleQuoteMatches);

Copy link
Member

Choose a reason for hiding this comment

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

is a regex ever not complex or error-prone? 😉

is there any measurable performance impact?

Copy link
Member Author

Choose a reason for hiding this comment

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

So - implemented and pushed now a fully working version.
However - yes, it seems like this currently has a huge impact on performance... So needs for sure some more love before it is ready to go. 😞

Signed-off-by: Jonas Rittershofer <[email protected]>
@jotoeri jotoeri force-pushed the enh/vue_translator_comments branch from b437a95 to 932b8d7 Compare March 22, 2021 22:26
@rakekniven
Copy link
Member

Any progress here? The translators will happily starting a party. At least me 😃

@jotoeri
Copy link
Member Author

jotoeri commented Apr 13, 2021

Ahh, I'm sorry, @rakekniven. I still have it on backlog & in my notifications, just recently had some other PRs to complete and due to exams at university not too much of a time for some hacking.
So it's not lost, i still want to complete that, i just need to take the time for. 😉

@rakekniven
Copy link
Member

rakekniven commented Oct 23, 2022

The last weeks I have seen many questions about context of strings based on vue files.

I am kindly asking @nickvergessen to have a look at this one.

Maybe @juliushaertl can spend some time on it as well?

@rakekniven rakekniven requested review from nickvergessen and removed request for MorrisJobke October 23, 2022 14:35
@skjnldsv
Copy link
Member

I would honestly be a little more comfortable with a bit of easy testing on this script 🙈

@rakekniven
Copy link
Member

@skjnldsv For sure anyone is invited and appreciated by all translators :-)

@rakekniven
Copy link
Member

Any update on this one?

@nickvergessen nickvergessen changed the title Allow vue-translator comments feat(translationtool): Allow vue-translator comments Apr 4, 2023
@juliusknorr juliusknorr removed their request for review May 15, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants