Implement the fn:generate-id function #641

egh · 2024-05-31T00:36:01Z

https://www.w3.org/TR/xpath-functions-31/#func-generate-id

Simple implementation, but it seems to work.

bundlemon · 2024-05-31T00:37:33Z

BundleMon

Files updated (2)

Status	Path	Size	Limits
✅	fontoxpath.esm.js	78.54KB (+99B +0.12%)	-
✅	fontoxpath.js	78.42KB (+98B +0.12%)	-

Total files change +197B +0.12%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

coveralls · 2024-05-31T00:42:56Z

coverage: 91.567% (+0.01%) from 91.556%
when pulling 2d9d9a5 on egh:implement-generate-id
into 35d0da5 on FontoXML:master.

DrRataplan · 2024-06-03T08:19:14Z

Thanks!!!

The test failing with a static error is this one:

  <test-case name="generate-id-018">
      <description>generate-id() applied to a sequence of element nodes in backwards-compatibility mode</description>
      <created by="Michael Kay" on="2010-12-20"/>
      <modified by="Michael Kay" on="2013-01-15" change="make dependency on XPath explicit"/>
      <environment ref="auction"/>
      <dependency type="spec" value="XP20+"/>
      <dependency type="feature" value="xpath-1.0-compatibility"/>
      <test>generate-id(//*) eq generate-id(/*)</test>
      <result>
         <assert-true/>
      </result>
   </test-case>

Sure. XPath 1.0 compatibility mode is not something we implement. Or ever will if it is up to me. Lets ignore it.

The others are stranger. I would expect them to work since they are just verifying all ids are distinct. Can you maybe look into that?

egh · 2024-06-03T20:53:20Z

@DrRataplan Thank you for looking at this! I wasn't sure if those xpath features were implemented (my xpath knowledge mostly stops at 2.0 😀 ). It turns out the counter wasn't properly being passed to some children. I believe I have corrected the issue.

egh · 2024-06-03T20:54:33Z

Looks like there are some type errors I need to fix...

egh · 2024-06-05T20:06:39Z

I just realized that this won't really work for my use case (an xslt processor) unless I allow the caller of an evaluateXpath function to pass in a counter. I'll give that a shot...

DrRataplan · 2024-06-05T20:17:38Z

Is that because you need the result to be deterministic over multiple calls? A weakmap on the module level should fit that, right? Otherwise, something on DomFacade would not be too dirty...

…

On Wed, 5 Jun 2024, 22:07 Erik Hetzner, ***@***.***> wrote: I just realized that this won't really work for my use case (an xslt processor) unless I allow the caller of an evaluateXpath function to pass in a counter. I'll give that a shot... — Reply to this email directly, view it on GitHub <#641 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGKEJEEUL2BVD3BJWHOE2TZF5VWLAVCNFSM6AAAAABIR3SHFWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJQHA3DKMBZGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

egh · 2024-06-07T14:32:06Z

Is that because you need the result to be deterministic over multiple calls? A weakmap on the module level should fit that, right? Otherwise, something on DomFacade would not be too dirty...

Yes, that's right... I think a DomFacade seems like the way to go. Thanks for the tip.

DrRataplan · 2024-06-11T14:34:45Z

src/expressions/DynamicContext.ts

@@ -16,6 +16,8 @@ class DynamicContext {
 	public contextItemIndex: number;
 	public contextSequence: ISequence;
 	public variableBindings: { [s: string]: () => ISequence };
+	public generateIdMap: WeakMap<Node, string>;


If you move this weakmap to the module level, as in, make it its own file like this:

const generateIdMap: WeakMap<Node,string> = new WeakMap(); export default generateIdMap;

and just directly talk to that in the generate-id function, circumventing the DynamicContext, you will get that stability over multiple calls. Because this is a weakMap I don't expect any memory downsides. I'd like this more than externalizing this state with a DomFacade. That would require other users of FontoXPath to do some upgrading on their side, whcih I'm trying to minimize.

Makes sense to me! I'll give this a shot. Thank you

I implemented the function using a module, but the WeakMap is not working as I expected with nodes. It doesn't seem to be recognizing previous nodes. I'm trying to track this down, but I would have expected nodes to be equal across subsequent calls. See the failing test here https://github.com/FontoXML/fontoxpath/actions/runs/9475980881/job/26108159273?pr=641#step:6:100

NodePointers. Forgot about them.

In XQuery (Update Facility) we are sometimes cloning nodes, a lot. When you wrap a large structure (like let $ele := <ele/> let $wrap := <wrap>{$ele}</wrap> return $wrap) the <ele/> in $ele and the one in the $wrap are not equal ($ele is $wrap/ele is false). playground

Figuring out how to circumvent this. The nodeCompare function compares node identity, so that's close. We might have to memoize the createPointer function in DomFacade.ts here. That is the only place we make NodePointers. This way it can work over XQuery-made nodes as well.

This seems to work for the tests I wrote. There are probably situations where it won't work:

import { NodePointer, TinyNode } from '../../domClone/Pointer'; import { ConcreteNode } from 'src/domFacade/ConcreteNode'; const generateIdMap: WeakMap<TinyNode | ConcreteNode, string> = new WeakMap(); let generateIdCounter = 0; function generateId(ptr: NodePointer): string { const node = ptr.node; if (!generateIdMap.has(node)) { generateIdMap.set(node, `id${++generateIdCounter}`); } return generateIdMap.get(node); } export default generateId;

Simple implementation, but it seems to work.

egh · 2024-06-19T17:55:02Z

I'm pretty happy with what I have here - what do you think?

DrRataplan · 2024-06-19T18:57:18Z

I think it looks great! I'll have to take a proper gander next week, if that is OK, after Midsommar. Thanks again!

…

On Wed, 19 Jun 2024, 19:55 Erik Hetzner, ***@***.***> wrote: I'm pretty happy with what I have here - what do you think? — Reply to this email directly, view it on GitHub <#641 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGKEJFH4UGSUNTAPTB6QNDZIHAYZAVCNFSM6AAAAABIR3SHFWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZZGI2DONBSGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

egh · 2024-06-20T00:23:31Z

No problem! Thank you. I just wanted to let you know it was ready for your review. Thanks for everything.

Better support depends on FontoXML/fontoxpath#641

DrRataplan

LGTM! Let's merge it!

DrRataplan · 2024-06-24T07:21:16Z

src/expressions/functions/builtInFunctions_node.ts

+) => {
+	let node: NodePointer;
+	if (!nodeValue) {
+		if (!dynamicContext || !dynamicContext.contextItem) {


You could've used the contextItemAsFirstArgument helper, that does this internally, but I'm fine either way.

And improve coverage

DrRataplan · 2024-07-12T15:22:19Z

Finally got around to those linting errors. Thanks again for the help!

DrRataplan · 2024-07-12T15:24:04Z

Since I'm no longer with Fonto I cannot release this. @bwrrp, can you hit the ship button somewhere after your holidays?

egh · 2024-07-12T16:28:09Z

Thank you @DrRataplan ! and best of luck with the new work.

bwrrp · 2024-08-13T12:41:18Z

Sorry for the delay, just released 3.33.0 which includes this

egh · 2024-08-13T16:23:07Z

🙏 Thank you!

DrRataplan requested changes Jun 11, 2024

View reviewed changes

egh force-pushed the implement-generate-id branch 5 times, most recently from 1d0a956 to 5f71b5b Compare June 13, 2024 00:13

Implement the fn:generate-id function

cf86458

Simple implementation, but it seems to work.

egh force-pushed the implement-generate-id branch from 5f71b5b to cf86458 Compare June 13, 2024 00:23

egh added a commit to egh/xjslt that referenced this pull request Jun 21, 2024

Basic support for xsl:key

462c64c

Better support depends on FontoXML/fontoxpath#641

DrRataplan previously approved these changes Jun 24, 2024

View reviewed changes

Remove some boilerplate by using contextItemAsFirstArgument

ba0e308

And improve coverage

DrRataplan dismissed their stale review via ba0e308 June 25, 2024 15:17

Add tests for uncovered lines

2d9d9a5

DrRataplan force-pushed the implement-generate-id branch from 6bd71eb to 2d9d9a5 Compare July 12, 2024 15:18

DrRataplan approved these changes Jul 12, 2024

View reviewed changes

DrRataplan merged commit 6a7bd94 into FontoXML:master Jul 12, 2024
8 checks passed

egh deleted the implement-generate-id branch July 12, 2024 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the fn:generate-id function #641

Implement the fn:generate-id function #641

egh commented May 31, 2024 •

edited

Loading

bundlemon bot commented May 31, 2024 •

edited

Loading

coveralls commented May 31, 2024 •

edited

Loading

DrRataplan commented Jun 3, 2024

egh commented Jun 3, 2024

egh commented Jun 3, 2024

egh commented Jun 5, 2024

DrRataplan commented Jun 5, 2024 via email

egh commented Jun 7, 2024

DrRataplan Jun 11, 2024

egh Jun 12, 2024

egh Jun 12, 2024

DrRataplan Jun 12, 2024

egh Jun 12, 2024

egh commented Jun 19, 2024

DrRataplan commented Jun 19, 2024 via email

egh commented Jun 20, 2024

DrRataplan left a comment

DrRataplan Jun 24, 2024

DrRataplan commented Jul 12, 2024

DrRataplan commented Jul 12, 2024

egh commented Jul 12, 2024

bwrrp commented Aug 13, 2024

egh commented Aug 13, 2024

Implement the fn:generate-id function #641

Implement the fn:generate-id function #641

Conversation

egh commented May 31, 2024 • edited Loading

bundlemon bot commented May 31, 2024 • edited Loading

BundleMon

coveralls commented May 31, 2024 • edited Loading

DrRataplan commented Jun 3, 2024

egh commented Jun 3, 2024

egh commented Jun 3, 2024

egh commented Jun 5, 2024

DrRataplan commented Jun 5, 2024 via email

egh commented Jun 7, 2024

DrRataplan Jun 11, 2024

Choose a reason for hiding this comment

egh Jun 12, 2024

Choose a reason for hiding this comment

egh Jun 12, 2024

Choose a reason for hiding this comment

DrRataplan Jun 12, 2024

Choose a reason for hiding this comment

egh Jun 12, 2024

Choose a reason for hiding this comment

egh commented Jun 19, 2024

DrRataplan commented Jun 19, 2024 via email

egh commented Jun 20, 2024

DrRataplan left a comment

Choose a reason for hiding this comment

DrRataplan Jun 24, 2024

Choose a reason for hiding this comment

DrRataplan commented Jul 12, 2024

DrRataplan commented Jul 12, 2024

egh commented Jul 12, 2024

bwrrp commented Aug 13, 2024

egh commented Aug 13, 2024

egh commented May 31, 2024 •

edited

Loading

bundlemon bot commented May 31, 2024 •

edited

Loading

coveralls commented May 31, 2024 •

edited

Loading