554/754 Simplify the new transitive-closure function #761
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaelhkay
added
XQFO
| @@ -18349,10 +18349,9 @@ return sort($in, $SWEDISH)</eg> | |||
|
|
|||
| <fos:function name="transitive-closure" prefix="fn"> | |||
| <fos:signatures> | |||
| <fos:proto name="transitive-closure" return-type="function(node()) as node()*"> | |||
| <fos:proto name="transitive-closure" return-type="node()*"> | |||
| <fos:arg name="start" type="node()?"/> | |||
There was a problem hiding this comment.
I believe that naming the parameter $node would be more consistent with the remaining spec. It seems obvious enough that it will be the node from which the operation starts. Next, it would be a helpful hint, as transitive closures could potentially be computed from data structures other than nodes.
| is unbounded. In this situation the function <var>TC</var> continues execution until no further nodes | ||
| are added to the result set. Note that if the <code>$step</code> function constructs new nodes, this | ||
| can lead to non-termination. Specifying an explicit value for <code>$max</code> guarantees termination.</p> | ||
| <eg><![CDATA[declare %private function tc-inclusive( |
There was a problem hiding this comment.
I’ve sllightly tweaked the formatting (and proactively renamed $start to $node):
declare %private function tc-inclusive(
$nodes as node()*,
$step as function(node()) as node()*
) as node()* {
let $nextStep := $nodes/$step(.)
let $newNodes := $nextStep except $nodes
return if (exists($newNodes))
then $nodes | tc-inclusive($newNodes, $step)
else $nodes
};
declare function transitive-closure(
$node as node(),
$step as function(node()) as node()*
) as node()* {
tc-inclusive($node/$step(.), $step)
};| <fos:expression><eg>let $tc := transitive-closure($direct-reports) | ||
| return $tc($data//person[@id="2"])/string(@id)</eg></fos:expression> | ||
| <fos:expression><eg>transitive-closure( | ||
| $data//person[@id="2"]), |
There was a problem hiding this comment.
Suggested change
| $data//person[@id="2"]), | |
| $data//person[@id="2"], |
| return $tc($data//person[@id="2"])/string(@id)</eg></fos:expression> | ||
| <fos:expression><eg>transitive-closure( | ||
| $data//person[@id="2"]), | ||
| $direct-reports)/string(@id)</eg></fos:expression> |
There was a problem hiding this comment.
Suggested change
| $direct-reports)/string(@id)</eg></fos:expression> | |
| $direct-reports | |
| )/string(@id)</eg></fos:expression> |
| @@ -18441,12 +18453,6 @@ return $tc($data)/@id/string()</eg></fos:expression> | |||
| return $tc($root) =!> document-uri()</eg> | |||
There was a problem hiding this comment.
The last example needs to be updated to match the new function signature:
transitive-closure($root, fn { document(//(xsl:import|xsl:include)/@href) })
=!> document-uri()
|
@michaelhkay Apart from the minor adjustments, I would approve the PR. |
michaelhkay
force-pushed
the
754-simplify-transitive-closure
branch
from
7ce3c03
to
2530ac7
Compare
ChristianGruen
added
the
Propose Merge without Discussion
|
The group agreed to merge this PR at meeting 054 |
ChristianGruen
removed
the
Propose Merge without Discussion
michaelhkay
added
Tests Added
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Drops the
$minand$maxparameters, with the effect that this corresponds much more closely to the general computer-science definition of a transitive closure.The function now computes the set of nodes delivered by the transitive closure of the supplied
$stepfunction when applied to a given$startnode, rather than returning a function item that must then be applied to the chosen $start node. This is hopefully easier for most users to understand, and does not lose any useful functionality.The
$minparameter of the old function is effectively forced to its default value of 1, and the $max value to its default of infinity.Fix #754
Fix #554
This PR addresses the main points of #554 in making the function correspond more closely to the mathematical (or at least the computer-science) definition of transitive closure. It doesn't implement other ideas in #554, like returning the depth of search alongside the actual closure. That's because I believe in the principle that wherever possible a function should do one thing in as simple a way as possible.