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

add XQuery test to check for missing entries in language files #454

Merged
merged 21 commits into from
Nov 8, 2024

Conversation

peterstadler
Copy link
Member

@peterstadler peterstadler commented Oct 15, 2024

Description, Context and related Issue

This PR introduces a new XQuery based check for missing entries in language files

Refs #154

How Has This Been Tested?

Checked locally by modifying language files and checking results of running ant check-language-files

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

@peterstadler peterstadler added this to the 1.0.0 milestone Oct 15, 2024
@peterstadler peterstadler linked an issue Oct 15, 2024 that may be closed by this pull request
@peterstadler
Copy link
Member Author

I've also added the new check "check-language-files" to the Github actions:

  1. The run https://github.com/Edirom/Edirom-Online/actions/runs/11352963364/job/31576942488 failed due to a missing entry in the German language file
  2. After adding the missing entry the Github action passed: https://github.com/Edirom/Edirom-Online/actions/runs/11353084552/job/31577350564

@peterstadler peterstadler self-assigned this Oct 15, 2024
@roewenstrunk
Copy link
Member

When I run the test, I get an error because I don't have the right libraries for ant: [java] Could not find net.sf.saxon.Query. Make sure you have it in your classpath.
Would you like to write a minimal description in the wiki of how this type of tests work and what requirements must be met?

@peterstadler
Copy link
Member Author

I have added some basic information regarding dependencies in the Readme:

Edirom-Online/README.md

Lines 104 to 110 in 3e57257

For running the tests provided in the [ANT build file] we rely on `xmllint`
and `SaxonHE`.
On a Debian based Linux system these can be installed with `apt-get install
libsaxonhe-java libxml2-utils`.
If SaxonHE is not available from your classpath by default you might need to
explicitly point ANT at it by providing the `-lib` parameter, e.g. `ant -lib
/usr/share/java/ run-all-tests`.

@peterstadler peterstadler force-pushed the 154-check-english-and-german-language-files branch from 3e57257 to faee0e0 Compare October 23, 2024 09:25
@peterstadler peterstadler force-pushed the 154-check-english-and-german-language-files branch from faee0e0 to 91794f3 Compare November 4, 2024 12:07
@peterstadler
Copy link
Member Author

pinging @bwbohl and @roewenstrunk for reviewing

@bwbohl
Copy link
Member

bwbohl commented Nov 6, 2024

Ok, I successfully tested the lang-test by doing the following:

  1. Use bwbohl/sencha-cmd as described in the README.

  2. Install missing dependecies for the tests

    apt-get update
    
    apt-get install libsaxonhe-java
    
  3. from within '/app' run the test, referencing the JARs and the buildfile

ant -lib /usr/share/java/ -buildfile testing/ant-tests.xml check-language-files

This reported nothing and when changing the name of one entry it reported the original term missing in the modified file and the modified term in the other file. Everything as expected, very nice!

bwbohl
bwbohl previously approved these changes Nov 6, 2024
Copy link
Member

@bwbohl bwbohl left a comment

Choose a reason for hiding this comment

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

Just a short follow-up: Should we also check add/data/xslt/i18n?

@peterstadler peterstadler force-pushed the 154-check-english-and-german-language-files branch from 91794f3 to f7ca4f5 Compare November 6, 2024 18:46
@bwbohl
Copy link
Member

bwbohl commented Nov 6, 2024

Thanks for imrproving!
Lot’s of discrepancies surfaced this way. Probably we should fix. them before merging ;-)

@bwbohl
Copy link
Member

bwbohl commented Nov 7, 2024

Just trying to fix the files and realised maybe we should also cover duplicates in the error report. Currently this would be the result for a duplicate entry in one file:

<key missing="" available="en de">csl</key>

@peterstadler
Copy link
Member Author

Regarding duplicates: Shall I add proper output for that case or is the current (undocumented) way of surfacing duplicates ok for now?

@bwbohl
Copy link
Member

bwbohl commented Nov 7, 2024

I’d prefer making it clean now, so e.g.

<key missing="" available="en de" duplicate="de">csl</key>

@peterstadler peterstadler force-pushed the 154-check-english-and-german-language-files branch from 7c0c01f to 3758100 Compare November 8, 2024 08:04
@peterstadler
Copy link
Member Author

added explicit warning for duplicate keys. Output looks like:

<results>
   <key duplicates="de en">controller.window.Window_renderingView</key>
   <key missing="en" available="de">global_execute</key>
   <key duplicates="de">controller.window.Window_summaryView</key>
   <key missing="de" available="en">global_execue</key>
</results>

@peterstadler peterstadler requested a review from bwbohl November 8, 2024 11:06
@bwbohl
Copy link
Member

bwbohl commented Nov 8, 2024

After retesting I have to report that it works fine for the files in locale but does not report duplicates in the new style when there are some in the ì18n` files.

Moreover if there˚s a duplicate in one of the locale files and another in the ì18nfiles only thelocale` gets reported.

testing/ant-tests.xml Outdated Show resolved Hide resolved
@peterstadler peterstadler force-pushed the 154-check-english-and-german-language-files branch from 644a754 to 4fc2890 Compare November 8, 2024 14:47
@peterstadler
Copy link
Member Author

Thank you very much @bwbohl for the thorough review!
In added your suggestions (great idea to run the tests in parallel!) and finally did some very explicit typing ;)

Works on my machine but it'd be great if you could have yet another (last) look.

@peterstadler peterstadler requested a review from bwbohl November 8, 2024 14:50
Copy link
Member

@bwbohl bwbohl left a comment

Choose a reason for hiding this comment

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

Added some minor code linting.
Works like a charm, great implementation work, love it!

@bwbohl bwbohl merged commit f807267 into develop Nov 8, 2024
4 checks passed
@bwbohl bwbohl deleted the 154-check-english-and-german-language-files branch November 8, 2024 15:49
Diginaut pushed a commit to korngold-werkausgabe/Edirom-Online_EWK-WA that referenced this pull request Dec 16, 2024
…m#454)

* add xquery to check for missing entries in language files

* add new check to ant build file

* some cleanup

* add new test to GIthub actions

* add dependency

* Install libsaxonhe-java for XQuery support

* add lib parameter to ant call

with path to Saxon-HE.jar, see https://packages.ubuntu.com/focal/all/libsaxonhe-java/filelist

* add missing entry to German language file

which already exists in the English version

* add information about dependencies for running tests

* fix typo

* remove duplicate xml:ids

* add support for language files at `/add/data/xslt/i18n`

* aligning i18n langfiles

* explicitly check for duplicate keys

* Update add/data/xslt/i18n/en.xml

Co-authored-by: Benjamin W. Bohl <[email protected]>

* Update add/data/xslt/i18n/en.xml

Co-authored-by: Benjamin W. Bohl <[email protected]>

* translate term

* fix grouping key for i18n files

Co-authored-by: Benjamin W. Bohl <[email protected]>

* run tests in parallel

Co-authored-by: Benjamin W. Bohl <[email protected]>

* be very specific about data types

* minor code linting

---------

Co-authored-by: bwbohl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Check english and german language files
3 participants