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

Upgraded to rdfjs #44

Merged
merged 1 commit into from
Dec 20, 2019
Merged

Conversation

LaurensRietveld
Copy link
Collaborator

See #35

What changed:

  • Upgraded to N3
  • Rdfjs terms instead of strings for query arguments (e.g. the searchTriples function)
  • Allow passing a datafactory as argument to fromFile
  • Returning rdfjs terms from the functions. Done by looping over the results

What still needs to be done after this PR:

  • Update README
  • Update type definitions

@RubenVerborgh
Copy link
Member

Thanks! Do you happen to have any insights in performance regressions?

Copy link
Member

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

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

Implementation looks excellent.

@RubenVerborgh
Copy link
Member

Pinging @mielvds and @rubensworks for a review.

Note: is semver major.

@LaurensRietveld
Copy link
Collaborator Author

Didn't do any testing yet (other than the test suite), so can't say much about the performance. Good thing is that the majority of the work in this pr (the test suite) is useful regardless of the solution (c vs js) we end up with.

Copy link
Member

@rubensworks rubensworks left a comment

Choose a reason for hiding this comment

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

LGTM

lib/hdt.js Show resolved Hide resolved
@RubenVerborgh RubenVerborgh merged commit a1c1218 into LinkedDataFragments:master Dec 20, 2019
@RubenVerborgh
Copy link
Member

Thanks a lot @LaurensRietveld. Leaving #45 to do some checks before we release.

@mielvds
Copy link
Collaborator

mielvds commented Dec 20, 2019

And update the docs...

@RubenVerborgh
Copy link
Member

@mielvds Yes, thx: #46

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants