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

Fix set semantics bug with other store interface #10

Merged
merged 2 commits into from
Feb 6, 2024

Conversation

maartyman
Copy link
Collaborator

@maartyman maartyman commented Jan 22, 2024

@rubensworks is this what you had in mind?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7609127000

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 7552705868: 0.0%
Covered Lines: 59
Relevant Lines: 59

💛 - Coveralls

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.

Yes, this looks perfect to me!

@surilindur could you try this out in link-traversal master and your void branch?

Comment on lines +10 to +13
subject: RDF.Term | null,
predicate: RDF.Term | null,
object: RDF.Term | null,
graph: RDF.Term | null,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
subject: RDF.Term | null,
predicate: RDF.Term | null,
object: RDF.Term | null,
graph: RDF.Term | null,
subject?: RDF.Term | null,
predicate?: RDF.Term | null,
object?: RDF.Term | null,
graph?: RDF.Term | null,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

It actually does support undefined: https://github.com/rdfjs/N3.js/blob/main/src/N3Store.js#L445-L447
But it appears the typings are not fully correct.

So yeah, let's keep it as-is for now. We can modify later once the N3 typings would get fixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assume this is support for when OTerm is null but the logic is the same for undefined.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, but since null is not the same as undefined, tsc will complain about it.

@surilindur
Copy link

surilindur commented Jan 22, 2024

This seems to produce the same results as the current 1.1.2 version in link traversal master branch, so it definitely does not seem to break that this time. I am also fairly sure I tested the correct versions of everything this time. Edit: also the tests in link traversal pass.

I will try to test with the VoID stuff soon, either today or tomorrow.

@surilindur
Copy link

After testing the version from this branch with the VoID-related work, it also seems to consistently produce the same results every time, with no spurious duplicates, since I cannot seem to be able to reproduce the cases where it was producing random numbers of results or exactly double the expected.

@rubensworks
Copy link
Member

Excellent, thanks for checking @surilindur, and thanks for the fix @maartyman!

@rubensworks rubensworks merged commit 4d0d540 into master Feb 6, 2024
13 checks passed
@rubensworks rubensworks deleted the fix-set-semantics-bug-with-other-interface branch February 6, 2024 07:40
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.

4 participants