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.

Sign up for GitHub

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

Jump to bottom

add in RDF dataset definitions from SPARQL query #50

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

add in RDF dataset definitions from SPARQL query #50

wants to merge 6 commits into from

Conversation

pfps
Copy link
Contributor

@pfps pfps commented Oct 31, 2024 •
edited by pr-preview bot
Loading

@pfps pfps requested a review from afs October 31, 2024 18:45
afs
afs reviewed Oct 31, 2024
View reviewed changes
spec/index.html Outdated
Comment on lines 1515 to 1523
<table>
<caption>Definition of active graph in an RDF dataset</caption>
<tr>
<td class="semantictable">
<p>The <dfn data-lt="active graph">active graph</dfn> in an RDF dataset is the graph from the dataset used for basic graph pattern
matching.</p>
</td>
</tr>
</table>
Copy link

Choose a reason for hiding this comment

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

This is SPARQL specific and can be left in SPARQL Query. It changes during query execution (GRAPH).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. I put it in just in case but I'll take it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@pfps pfps requested a review from doerthe October 31, 2024 20:10
@domel
Copy link
Contributor

domel commented Oct 31, 2024

Why definitions are in tables? Such text certainly has nothing to do with the table.

@pfps
Copy link
Contributor Author

pfps commented Oct 31, 2024

Why definitions are in tables? Such text certainly has nothing to do with the table.

Just because all the definitions were done that way by Pat for 1.1.

gkellogg
gkellogg requested changes Oct 31, 2024
View reviewed changes
Copy link
Member

@gkellogg gkellogg left a comment

Choose a reason for hiding this comment

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

RDF dataset is defined in RDF Concepts, not Semantics; it is RDF dataset merge which should be defined here.

Note that some of the markup is wrong (data-lt overrides the text content of the element, which is not always what you want).

spec/index.html Outdated
<caption>Definition of an RDF dataset</caption>
<tr>
<td class="semantictable">
<p>An <dfn data-lt="RDF dataset">RDF dataset</dfn> I is a set:</p>
Copy link
Member

Choose a reason for hiding this comment

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

RDF Dataset is actually defined in RDF Concepts, although it has an alias here, so it's sufficient to just use an anchor.

Suggested change
<p>An <dfn data-lt="RDF dataset">RDF dataset</dfn> I is a set:</p>
<p>An <a>RDF dataset</a> I is a set:</p>
```suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed in latest commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dfn markup is copied from examples elsewhere. If they are wrong here then I think there are lots that have to be changed throughout the document set.

Copy link
Member

Choose a reason for hiding this comment

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

This document is more complicated than most as the form is to "define" terms which an also defined in RDF Concepts. The <dfn data-cite="...">...</dfn> form is there to be able to use these terms as if they're defined in RDF Semantics while referencing the version in RDF Concepts. Note that data-lt provides an alternative way to reference the same definition, but it can also affect the term which is "exported" from the document. It can be useful to look at the Terms defined by this specification section to see what terms are actually defined here.

I did scan the other definitions in the document and they seem correct to me.

spec/index.html Outdated Show resolved Hide resolved
@afs
Copy link

afs commented Nov 1, 2024 •
edited
Loading

RDF dataset is defined in RDF Concepts, not Semantics; it is RDF dataset merge which should be defined here.

We need notation for "RDF dataset" for use in SPARQL, and in the definitions of "RDF Dataset Merge".
RDF Concepts does not provide that notation at the moment.

If it were in RDF concepts, quoting here (without the heading "Definition of an RDF dataset") would help the reader. SPARQL puts in the notation for RDF Dataset - and a SPARQL Query PR suggests putting that in a box.

The section RDF Concepts section RDF Dataset Comparison would also benefit (as it is currently, it can be read as only applying to datasets with one named graph).

gkellogg
gkellogg reviewed Nov 1, 2024
View reviewed changes
spec/index.html Outdated Show resolved Hide resolved
@pfps @gkellogg
Update spec/index.html
13842cf 
Co-authored-by: Gregg Kellogg <[email protected]>
gkellogg
gkellogg approved these changes Nov 2, 2024
View reviewed changes
Copy link
Member

@gkellogg gkellogg left a comment

Choose a reason for hiding this comment

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

I pushed some changes for "merge" (now dfn-rdf-graph-merge) and duplicate definitions for "RDF dataset" directly to the branch.

@afs afs added the spec:enhancement Change to enhance the spec without affecting conformance (class 2) –see also spec:editorial label Nov 3, 2024
@pfps
Copy link
Contributor Author

pfps commented Nov 7, 2024

This looks to be ready to go? Any objections?

@afs
Copy link

afs commented Nov 8, 2024 •
edited
Loading

Do we want the title to be "Definition of an RDF dataset" when the definition is in RDF concepts?

It could called something like "Description of an RDF Dataset"; alternatively, consider it a parallel definition and keep as it is currently "Definition of an RDF dataset".

@pfps
Copy link
Contributor Author

pfps commented Nov 20, 2024

Perhaps "Formal Definition" would work?

@afs
Copy link

afs commented Nov 21, 2024 •
edited
Loading

Perhaps "Formal Definition" would work?

As in a "squiggly mathy definition"? 😄 .

The question will then be why is the merge definition not "formal"?

The current "Definition of an RDF dataset" is better than "Formal Defintion" IMO.

Let's put it in as-is pending other suggestions.

@pfps
Copy link
Contributor Author

pfps commented Nov 21, 2024

I think the right thing is to completely defer to RDF Concepts for the definition of an RDF dataset. This means adding it to the imported terminology section at the beginning and then making the "definition" in the new section just an alternative way of saying that definition.

@pfps
Copy link
Contributor Author

pfps commented Nov 21, 2024

The mess with the HTML PR needs to be resolved before this PR is updated, I think.

@afs afs mentioned this pull request Dec 18, 2024
Open
1 task
@afs
Copy link

afs commented Dec 22, 2024

The mess with the HTML PR needs to be resolved before this PR is updated, I think.

The HTML on main has been adjusted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afs afs afs approved these changes

@gkellogg gkellogg gkellogg approved these changes

@doerthe doerthe Awaiting requested review from doerthe

Assignees
No one assigned
Labels
spec:enhancement Change to enhance the spec without affecting conformance (class 2) –see also spec:editorial
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pfps @domel @afs @gkellogg