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

Implement otherwise and combinating tests from XQ4 #660

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

DrRataplan
Copy link
Collaborator

No description provided.

@DrRataplan DrRataplan requested a review from bwrrp October 26, 2024 08:59
Copy link

bundlemon bot commented Oct 26, 2024

BundleMon

Files updated (2)
Status Path Size Limits
fontoxpath.esm.js
79.39KB (+724B +0.9%) -
fontoxpath.js
79.27KB (+722B +0.9%) -

Total files change +1.41KB +0.9%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@DrRataplan DrRataplan force-pushed the xquery-next branch 2 times, most recently from 9336e4c to b2d34d4 Compare October 26, 2024 09:29
Copy link
Member

@bwrrp bwrrp left a comment

Choose a reason for hiding this comment

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

Nice to see some 4.0 features! Left a few smaller comments.

Not sure what's going on with the tests though, most seem to simply require an update of the expected error, but something might have gone wrong with function calls?

"coverage": "nyc report --reporter=text-lcov",
"integrationtests": "ts-mocha --require test/testhook.js \"test/specs/parsing/**/*.ts\" --parallel",
"integrationtests": "ts-mocha --require test/testhook.js \"test/specs/parsing/**/*.ts\"",
Copy link
Member

Choose a reason for hiding this comment

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

was this intentional or removed for debugging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Intentional. the parallel hides easy to fix errors behind confusing errors. It's CI, so a bit slower is fine

src/expressions/tests/CombinatingTest.ts Outdated Show resolved Hide resolved
src/expressions/tests/CombinatingTest.ts Outdated Show resolved Hide resolved
src/expressions/util/Bucket.ts Show resolved Hide resolved
@@ -84,7 +84,7 @@ function parseXPath(xpathExpression: EvaluableExpression): Expression {

const ast =
typeof xpathExpression === 'string'
? parseExpression(xpathExpression, { allowXQuery: false })
? parseExpression(xpathExpression, { allowXQuery: false, version: 4 })
Copy link
Member

Choose a reason for hiding this comment

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

Should this depend on the options?

In that case we'd probably also need separate caches for the 3.1 vs 4 expressions.

src/jsCodegen/compileXPathToJavaScript.ts Outdated Show resolved Hide resolved
src/parsing/compileAstToExpression.ts Outdated Show resolved Hide resolved
src/parsing/prscParser.ts Outdated Show resolved Hide resolved
src/parsing/prscParser.ts Outdated Show resolved Hide resolved
src/parsing/prscParser.ts Outdated Show resolved Hide resolved
Unsure why this is not wrong, but it makes sense: the unminified fontoxpath is used alongside the
minified one. These have different state (registered functions)
@coveralls
Copy link

coveralls commented Nov 22, 2024

Coverage Status

coverage: 91.545% (-0.03%) from 91.575%
when pulling b520bac on xquery-next
into 40e08de on master.

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.

3 participants