Skip to content

Fix context processing for reverse terms #566

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

Merged
merged 6 commits into from
Jul 20, 2023

Conversation

niklasl
Copy link
Contributor

@niklasl niklasl commented Jun 7, 2023

This changes the algorithm for processing a reverse term so that it doesn't return early. Fixes #565.

Includes one test for expansion using a reverse term with @container: @index and @index set to a property, and a roundtripping compaction test based on the former.

(Note: the round-tripping compaction test reuses the expand test data. That does not seem to be the practise; is there any rationale against that?)


Preview | Diff

@davidlehn
Copy link
Contributor

Does this need parallel toRdf tests too? t0131 is not going to line up between expand/toRdf since there already exists some toRdf tests over 0130. Not sure if some other prefix would be better here for that reason alone.

@gkellogg
Copy link
Member

Yes to @davidlehn’s points. Also, there should be an entry in the “changes since Recommendation …” section.

@niklasl
Copy link
Contributor Author

niklasl commented Jun 28, 2023

Yes to @davidlehn’s points. Also, there should be an entry in the “changes since Recommendation …” section.

Added an entry about this change in 8bfdf28.

@niklasl
Copy link
Contributor Author

niklasl commented Jun 28, 2023

Does this need parallel toRdf tests too? t0131 is not going to line up between expand/toRdf since there already exists some toRdf tests over 0130. Not sure if some other prefix would be better here for that reason alone.

Added toRdf#t0133 in fb521b7.

The numbering of "non-prefixed" toRdf tests don't seem to correlate with neither expand nor compact test numbering, so I just continued on the non-prefixed sequence rather than using any "prefix". (The "prefixed" ones did seem to correlate, but I didn't see any obvious naming to use.)

@niklasl
Copy link
Contributor Author

niklasl commented Jun 28, 2023

Note that I added textual references in the purpose text to the original expand test id. We should consider adding proper interlinking of all tests instead, so we can query on what is based on what, and if there are any missing round-trip or to/from RDF tests. That would be a separate task of course.

@gkellogg gkellogg requested review from dlongley and pchampin July 10, 2023 20:37
Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

This seems ok to me and like an oversight previously.

Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

Trivial, but important.

Co-authored-by: Ted Thibodeau Jr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The algorithm for processing a reverse term returns too early
7 participants