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

Translation consistency checking #1458

Merged
merged 9 commits into from
Jul 24, 2024
Merged

Conversation

eric-gade
Copy link
Collaborator

@eric-gade eric-gade commented Jul 22, 2024

What does this PR do? 🛠️

This PR tries hard to implement the requirements of #1429, specifically:

  • Ensuring that we have translation terms in the .po files for every t() variant called throughout our templates/code
  • Reporting on any potentially "stale" translations in the .po files that are unused anywhere in code

closes #1429

What does the reviewer need to know? 🤔

You can run the consistency check with make check-translations or make ct for short.

The check will fail with an error status if there are any t() or | t calls in the code/templates, along with reporting on which files/lines these calls appear.

It will also spit out a warning -- but not exit with failure -- if it believes there are "stale" translations in the translation file. I have decided to make this a warning rather than a requirement because we might want to keep certain translations around for as yet unspecified reasons. In particular, we have some in there now for the CMS that would not be found by the current setup.

Implementation

The check is a custom script that can be found inside of tests/translations.

A series of regex checks across templates and php files is conducted, and then checked against the values found in the .po translation files for each language.

You can specify what template/code files to include or exclude in the tests/translations/config.js file.

Fair warning

At this point I'm requesting a pull of this code because it could be useful when updating translations. Without having done true translations, it's unclear whether these checks will be "correct" in most cases. Therefore I have decided not to add these checks to the CI for now.

Also: there's lots of new code here. After banging my head against this for several days, it's still unclear to me whether this is all worth it, and I would not be offended if we scrapped it altogether for now.

@eric-gade eric-gade marked this pull request as ready for review July 22, 2024 19:16
@eric-gade eric-gade force-pushed the eg-translations-testing-experiments branch 2 times, most recently from 79e82e8 to c729793 Compare July 22, 2024 19:29
Copy link
Collaborator

@greg-does-weather greg-does-weather left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines 13 to 21
'views-mini-pager.html.twig',
'pager.html.twig',
'menu-local-tasks.html.twig',
'breadcrumb.html.twig',
'maintenance-page.html.twig',
'field--comment.html.twig',
'filter-tips.html.twig',
'mark.html.twig',
'block--local-tasks-block.html.twig'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these files that we copied directly from the parent theme and haven't edited? Not something to do in this PR, but maybe we should have a debt ticket to delete the template files we haven't modified since (I think) Drupal will pull them from the parent theme automatically if we don't have them.

Comment on lines 19 to 23
const phpPaths = config.php.include.reduce((prev, current) => {
const relativeGlob = path.resolve(__dirname, current);
const filePaths = globSync(relativeGlob);
return prev.concat(filePaths);
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

When this is merged, we should open a corresponding debt ticket to add exclusion handling to the PHP paths. The config file has a placeholder for them and I suspect a future version of us might add an exclusion and be perplexed when it doesn't work. 😂

Comment on lines 128 to 135
const getLineNumberForPosition = (source, position) => {
let cursor = 0;
const lines = source.split("\n");
for(let i = 0; i < lines.length; i++){
const currentLine = lines[i];
cursor += currentLine.length + 1; // Add the newline char
if(position <= cursor){
return i + 1; // Editors use index-1 for line counting
}
}

return -1;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

--Note
Not picking up nearly all of the instances across templates. This is
probably because of the regex(es) I have made. Need to revisit.
Test case is the points page, which is not being picked up at all
Initial php side structure (attempt)
Updating JS tests for translations
Removing PHP side attempt code
Moving package dependencies and updating Makefile
@eric-gade eric-gade force-pushed the eg-translations-testing-experiments branch from 2efd3f4 to d9a6eb9 Compare July 24, 2024 19:13
@eric-gade eric-gade merged commit 75f76b7 into main Jul 24, 2024
17 checks passed
@eric-gade eric-gade deleted the eg-translations-testing-experiments branch July 24, 2024 19:56
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.

Create tests for translation file consistency
2 participants