FontoXML / fontoxpath

A minimalistic XPath 3.1 implementation in pure JavaScript
MIT License
135 stars 17 forks source link

Implement the fn:generate-id function #641

Closed egh closed 4 months ago

egh commented 6 months ago

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

Simple implementation, but it seems to work.

bundlemon[bot] commented 6 months ago

BundleMon

Files updated (2) Status | Path | Size | Limits :------------: | ------------ | :------------: | :------------: :white_check_mark: | fontoxpath.esm.js
| 78.54KB (+99B +0.12%) | - :white_check_mark: | fontoxpath.js
| 78.42KB (+98B +0.12%) | -

Total files change +197B +0.12%

Final result: :white_check_mark:

View report in BundleMon website ➡️


Current branch size history | Target branch size history

coveralls commented 6 months ago

Coverage Status

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

DrRataplan commented 5 months ago

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 commented 5 months ago

@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 :grinning: ). It turns out the counter wasn't properly being passed to some children. I believe I have corrected the issue.

egh commented 5 months ago

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

egh commented 5 months ago

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 commented 5 months ago

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 https://github.com/FontoXML/fontoxpath/pull/641#issuecomment-2150865092, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGKEJEEUL2BVD3BJWHOE2TZF5VWLAVCNFSM6AAAAABIR3SHFWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJQHA3DKMBZGI . You are receiving this because you were mentioned.Message ID: @.***>

egh commented 5 months ago

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.

egh commented 5 months ago

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

DrRataplan commented 5 months ago

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 https://github.com/FontoXML/fontoxpath/pull/641#issuecomment-2179247424, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGKEJFH4UGSUNTAPTB6QNDZIHAYZAVCNFSM6AAAAABIR3SHFWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZZGI2DONBSGQ . You are receiving this because you were mentioned.Message ID: @.***>

egh commented 5 months ago

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

DrRataplan commented 4 months ago

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

DrRataplan commented 4 months ago

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

egh commented 4 months ago

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

bwrrp commented 3 months ago

Sorry for the delay, just released 3.33.0 which includes this

egh commented 3 months ago

:pray: Thank you!