Skip to content

Allow attributes to be passed as arguments to custom functions#685

Merged
bwrrp merged 1 commit into
FontoXML:masterfrom
egh:allow-attribute-args-for-functions
Jun 30, 2026
Merged

Allow attributes to be passed as arguments to custom functions#685
bwrrp merged 1 commit into
FontoXML:masterfrom
egh:allow-attribute-args-for-functions

Conversation

@egh

@egh egh commented May 26, 2026

Copy link
Copy Markdown
Contributor

I'm not sure why this error checking was there: from what I can tell it works fine to pass an attribute into a custom function and the code that is called to transform the xpath value into a javascript value seems to handle attributes: https://github.com/egh/fontoxpath/blob/allow-attribute-args-for-functions/src/transformXPathItemToJavascriptObject.ts#L140

@bundlemon

bundlemon Bot commented May 26, 2026

Copy link
Copy Markdown

BundleMon

Files updated (2)
Status Path Size Limits
fontoxpath.js
79.6KB (-28B -0.03%) -
fontoxpath.esm.js
79.71KB (-29B -0.04%) -

Total files change -57B -0.03%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@egh egh force-pushed the allow-attribute-args-for-functions branch from aac2148 to c1145e3 Compare May 26, 2026 06:47
@coveralls

coveralls commented May 26, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 91.54% (-0.002%) from 91.542% — egh:allow-attribute-args-for-functions into FontoXML:master

@DrRataplan DrRataplan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Coverage dropped marginally because there are fewer branches. Makes sense.

Let's just reuse the older test as a regression test for the future.

Thanks @egh! Must've been a fun puzzle!

});

it('disallows attributes as parameters', () => {
chai.assert.throws(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we reuse this test as 'does allow attributes as parameters'?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll reverse the logic!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done and force-pushed changes.

@DrRataplan

Copy link
Copy Markdown
Contributor

I have a vague recollection we had this because something in fontonwas wrong with attributes. Maybe this stems from before we had attribute nodes in blueprints?

@egh

egh commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

@DrRataplan Thanks for the review. I should have added more context. In xjslt I was trying to process a stylesheet which passed attribute nodes as arguments to the key function (https://github.com/ncbi/JATSPreviewStylesheets/blob/master/xslt/main/jats-html.xsl#L3267). This was throwing an error. I worked around it for a while by wrapping @id in string(@id). But I decided I should look into the underlying issue. I couldn't figure out a way to work around the issue, so I took a look at the fontoxpath code. I found this check. Once I removed the check, everything worked fine (as far as I can tell). I don't know the fontoxpath code that well, but since the tests seem to pass, I thought it would be fine to submit a PR.

@egh egh force-pushed the allow-attribute-args-for-functions branch from c1145e3 to 12c848d Compare May 26, 2026 17:06
@DrRataplan

Copy link
Copy Markdown
Contributor

@bwrrp do you see a moment to review / merge this PR?

@bwrrp bwrrp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Apologies for the delay, looks good to me! Merging

@bwrrp bwrrp merged commit 7506ae0 into FontoXML:master Jun 30, 2026
7 checks passed
@egh

egh commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Thank you @bwrrp !

@egh egh deleted the allow-attribute-args-for-functions branch June 30, 2026 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants