Re-enable reverted changes

comunica / sparqlee

⚙️ SPARQL expression evaluator library - Moved to @comunica/expression-evaluator

https://www.npmjs.com/package/sparqlee

14 stars 7 forks source link

Re-enable reverted changes #124

Closed jitsedesmet closed 3 years ago

jitsedesmet commented 3 years ago

This pull request aims to re-enable the reverted changes introduced in #122 . It seemed like the problem was with @rubensworks his dist directory. Some files in that directory where expected to be deleted but weren't. This caused some vague error messaging. This pull request just merges the branch reverted/jitse. I checked:

Comunica test
Comunica spec
Comunica integration
Sparqlee test
sparqlee benchmark

This all seemed fine.

I also looked at the 2 commits that where performed on master, and caused merge issues (2224414f2cd1d8afdea5380d70fdf661a8caf7b7, 0170205f6a06c4125138a6730f0ca736f65e874e). The changes performed in these commits are still present in this PR. (Are not changed to the original)

jitsedesmet commented 3 years ago

It seems like some CI tests fail. Is it possible this is a problem with the CI?

rubensworks commented 3 years ago

That's a weird error, let me restart it...

coveralls commented 3 years ago

Coverage increased (+1.08%) to 89.341% when pulling ff68adc7b57fc36c0f06b6490bf7eef0fd5ce8a3 on jitsedesmet:bugfix/re-enable-reverted into 2224414f2cd1d8afdea5380d70fdf661a8caf7b7 on comunica:master.

rubensworks commented 3 years ago

Tests pass now :-)

It's a bit unfortunate that we loose the commit history with this though, as it's all squashed in a single commit. Any chance you could just do a plain merge and preserve the original 13 commits?

jitsedesmet commented 3 years ago

That's indeed a problem. If this is possible within git could you provide some more information? I fear my git knowledge is not that great.

rubensworks commented 3 years ago

Should just be a matter of checking out master, creating a new branch from that, and git merge-ing reverted/jitse into it (which may cause some merge conflicts that can then be resolved via a merge commit).

jitsedesmet commented 3 years ago

It is really strange the history wasn't merged as well. I tried again and this time it seems to have worked correctly. I will close this PR in favor of the new one (#125).