fontoxpath icon indicating copy to clipboard operation
fontoxpath copied to clipboard

Implement the fn:lang function

Open egh opened this issue 1 year ago • 3 comments

  • Implemented the lang function.
  • Added some basic tests for this
  • Not sure how to properly run the QT3 test suite to test this - I ran the test suite and everything passed, but with pending tests.

egh avatar May 22 '24 13:05 egh

BundleMon

Files updated (2)
Status Path Size Limits
:white_check_mark: fontoxpath.esm.js
78.44KB (+232B +0.29%) -
:white_check_mark: fontoxpath.js
78.33KB (+229B +0.29%) -

Total files change +461B +0.29%

Final result: :white_check_mark:

View report in BundleMon website ➡️


Current branch size history | Target branch size history

bundlemon[bot] avatar May 22 '24 14:05 bundlemon[bot]

Hey Erik,

Thanks so much! I'll fix some small things when I get home again, but to answer your questions:

  • The QT3 tests can be done using the command npm run alltests -- --regenerate. This will empty out the known-failing list and refill it with new errors. It takes some time though, so make sure you can get a coffee while waiting :wink:
  • But before that, the fn-lang-related tests should be marked as possibly running. You can do that here: https://github.com/FontoXML/fontoxpath/blob/master/test/assets/runnableTestSets.csv#L114. Just set it to true before running the tests.
  • I see some small linting / formatting errors. Pesky tabs versus spaces. A npm run lint-fix should resolve that.

I'll give it a review when I get home, but looks great! And thanks so much for the tests!

Kindest regards,

Martin

DrRataplan avatar May 22 '24 14:05 DrRataplan

Coverage Status

coverage: 91.556% (-0.04%) from 91.599% when pulling df4f6b5bbf1a290bc6b76e637012f01eb814b3c6 on egh:implement-fn-lang into b05b9a526466cc62ba43f0ac98d48debc967980c on FontoXML:master.

coveralls avatar May 22 '24 14:05 coveralls

Hey Erik,

Thanks so much! I'll fix some small things when I get home again, but to answer your questions:

* The QT3 tests can be done using the command `npm run alltests -- --regenerate`. This will empty out the known-failing list and refill it with new errors. It takes some time though, so make sure you can get a coffee while waiting 😉

* But before that, the fn-lang-related tests should be marked as possibly running. You can do that here: https://github.com/FontoXML/fontoxpath/blob/master/test/assets/runnableTestSets.csv#L114. Just set it to `true` before running the tests.

* I see some small linting / formatting errors. Pesky tabs versus spaces. A `npm run lint-fix` should resolve that.

I'll give it a review when I get home, but looks great! And thanks so much for the tests!

I fixed the linting problems, enabled & ran the QT3 tests, and fixed the bugs it revealed. There are 2 outstanding lang tests that are failing, but I frankly don't understand the xpath so I'm sure what is going wrong.

egh avatar May 22 '24 23:05 egh

Hey Erik,

Thanks! The test that is still failing is of the shape let $l := /langs/para[4]!fn:lang#1 return /langs/para[1]!$l('de'). I think it's trying to bind the context item of paragraph 4 to the fn:lang function. I'll have to look further into that, but it looks like the function binding is misbehaving here, rather than the lang function.

This does not block merging.

I'll wait until the pipeline is done and I'll merge it!

Thanks again!

Martin

DrRataplan avatar May 23 '24 07:05 DrRataplan

Merged! @bwrrp can you run an npm release sometime this or next week? 🙏

DrRataplan avatar May 23 '24 12:05 DrRataplan