-
Notifications
You must be signed in to change notification settings - Fork 2
Linked function names of algebra operators, and markup of variables in Section 18.5, 18.5.1, and 18.5.2 #216
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
Conversation
…5 and 18.5.1, but not yet in 18.5.2
I will resist the temptation to read the actual content and revise for comprehension. :-) I look forward to the near-future PR which does "something similar for the operators of the algebraic syntax", which actual content I will also ignore. Then I will make my own review of the actual content of Sections 18.5 SPARQL Algebra, 18.5.1 Aggregate Algebra, and 18.5.2 Evaluation Semantics. |
@TallTed I think there is plenty to revise. I have a bunch of ideas for improving the presentation of the formal parts, but my intention is to keep this separate from pure markup improvements to keep the PRs focused. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for this work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great and a good improvement.
I do find the linking "eval" as less useful because it occurs everywhere, several times, to the point of being a distracting.
However, it is the same HTML in the document. Possibly we could style it differently (e.g. no underline) at some point.
@afs I admit, I find the underlining of the now-linked (as per this PR) function names a bit ugly and distracting as well. The explicit CSS classes introduced by this PR do indeed allow us to style these particular links differently, removing the underlining. Having said that, linking all the occurrences of the eval function to the start of the recursive definition of this function is useful, I think; in particular, because there is another function called "eval" in the spec, namely the one for Property Path patterns in Section 18.4 Property Path Patterns. |
</div> | ||
<div class="defn"> | ||
<p><b>Definition: <span id="defn_algOrdered">OrderBy</span></b></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to be losing existing anchors in this change. Could we preserve id
values in adjacent markup (the enclosing <p>
or <b>
perhaps) to allow any existing deep links into the 1.1 spec to still work with the 1.2 spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elsewhere, and in other docs, there are empty span's e.g.<span id=><!-- comment --></span>
or <span id=/>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to be losing existing anchors in this change. [...]
I checked that these anchors are not used anywhere in the spec, but you are right, someone else may have used them externally. I have restored them now (commit 38052ef) using empty <span>
elements as we have already done elsewhere for other obsolete anchors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kasei with the anchors restored now, do you approve this PR?
This makes me think that one of these functions should be renamed, and if any other names are used for multiple functions, that all but one of each such group should be renamed. |
It would be possible to call the other one "evalPath". |
One thing that has always bugged me is that the spec represents the operators of the abstract/algebraic syntax of SPARQL in exactly the same way as it represents the algebra functions used to define the evaluation semantics. As an example, consider the following formula from Section 18.5.2 Evaluation Semantics.
The 'Filter' on the left-hand side of this formula represents an operator of the algebraic syntax (as introduced in the second table of Section 18.2 Translation to the SPARQL Algebra), whereas the 'Filter' on the right-hand side refers to the algebra operator defined in Section 18.5 SPARQL Algebra. As you can see, they look identical.
This has often been a source of confusion for me. While the formulas of the form as above become understandable once one has figured it out (which is not trivial for someone new coming to the spec), it is worse for cases in which something like 'Filter( ... )' is mentioned elsewhere, in particular just within text.
This PR is a first step towards fixing this issue. All mentions of the algebra operators are wrapped as links to their corresponding definition in Section 18.5 SPARQL Algebra, and these links have a dedicated CSS
class
attribute that allows us to make them visually different in the future. Additionally, the PR adds another suchclass
attribute for the (earlier-introduced) links to the set functions, and it adds<var>..</var>
markup for all formulas in Sections 18.5 SPARQL Algebra, 18.5.1 Aggregate Algebra, and 18.5.2 Evaluation Semantics. The example formula above looks as follows now.I am leaving it to a future PR to do something similar for the operators of the algebraic syntax (e.g., the 'Filter' on the left-hand side of this formula) in order to avoid making this PR too complex. For the same reason, this PR does not change any of the actual content.
Preview | Diff