comunica / sparqlee

⚙️ SPARQL expression evaluator library - Moved to @comunica/expression-evaluator
https://www.npmjs.com/package/sparqlee
14 stars 7 forks source link

Feature/auto casting function arguments #103

Closed jitsedesmet closed 3 years ago

jitsedesmet commented 3 years ago

This pull request aims to provide an implementation on auto casting functions as talked about in comunica/sparqlee#102 . This pull request should NOT be merged before the scheme talked about in comunica/sparqlee#102 is added.

There are a few questions to be answered before merging this PR too, those questions should be marked with a TODO.

Edit: We can close a few issues with this PR: comunica/sparqlee#13, comunica/sparqlee#94 and comunica/sparqlee#103. When merging we open issues about: cast decimal so we can close comunica/sparqlee#12 , questions surrounding arithmeticWidening, revision of https://github.com/comunica/sparqlee/pull/103#discussion_r687755303 (I think this issue is mainly for @wschella) and an issue surrounding caching and open world types. The PR also makes implementation of comunica/sparqlee#87 a lot easier.

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 1138484875


Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/functions/Helpers.ts 26 27 96.3%
lib/functions/NamedFunctions.ts 3 4 75.0%
lib/functions/RegularFunctions.ts 8 9 88.89%
lib/functions/SpecialFunctions.ts 36 49 73.47%
<!-- Total: 291 307 94.79% -->
Totals Coverage Status
Change from base Build 1128445387: 2.4%
Covered Lines: 1389
Relevant Lines: 1499

💛 - Coveralls
jitsedesmet commented 3 years ago

Therefore, I would recommend to add more detailed unit tests to the parts of the code you touched. Should be straightforward to do, now that this code is still fresh.

Do you think about changing the test directory structure at the same time? Like talked about in comunica/sparqlee#34 ? So we can create a dir for unit tests as well?

rubensworks commented 3 years ago

Do you think about changing the test directory structure at the same time? Like talked about in comunica/sparqlee#34 ? So we can create a dir for unit tests as well?

Oh, I thought we did that one already 😅

I think that's quite urgent, yes. But I would do it in a separate PR.

wschella commented 3 years ago

I can review the PR this afternoon. I think changing the test folder structure can be done & and merged quickly. Then this PR can be rebased on it (since it will be a significant one, and I do really propose fixing everything mentioned in comunica/sparqlee#102 at once here, it's too closely related). We should indeed strive to not decrease test coverage here, but everything should be able to be tested in theory with some entries in a TestTable somewhere.

jitsedesmet commented 3 years ago

When merging this PR we should also open 2 new issues.

  1. Is arithmetic widening the right choice? This PR creates a function arithmetic widening this function copy's previous behavior. We should check if this behavior is spec compliant and add links to that part of the spec. Reason: it could be that short + short \ne short. Example: is 240 + 100 still a short? It can not be represented as a short. Should we check this? Or do we need to return a short with value 340 which is not a valid short value.
  2. The requested change by @wschella right here https://github.com/comunica/sparqlee/pull/103#discussion_r687755303 . Should be checked closer. resolving that issue should also provide an explanation on any choices made there.
jitsedesmet commented 3 years ago

If we wish to implement https://www.w3.org/TR/xpath-31/#promotion completely, we should make sure we cast the decimal to double or float when promoting decimal. I will leave this for now. I think we should open a new issue adding this (I think small) feature. I feel like we better do this in a new issue because we might need a package for this? I'm not sure though

jitsedesmet commented 3 years ago

Thank you!

Before we merge, can you again double-check if all tests (unit, integration, spec, ...) still pass in Comunica?

I checked the overal tests and ran test, integration and spec in the actor-init-sarql package. All tests succeeded.

I will add the closing issues.

rubensworks commented 3 years ago

@wschella Merged this one already. Feel free to open issues in case you still see issues with this.