search

eXist-db / shared-resources

An app package with shared resources used by several eXist-db applications
5 stars 16 forks source link

Allow 20 items in a template rather than 15. #27

Closed craigberry closed 6 years ago

craigberry commented 6 years ago

Either number is arbitrary, but we really needed 16 for a specific application, so move the limit out a bit.

adamretter commented 6 years ago

@craigberry I think we could remove the arbitary limit here by either:

  1. switching to use fn:apply - https://www.w3.org/TR/xpath-functions-31/#func-apply
  2. recursively currying the function using partial function application until we run our of arguments.

I am not sure if eXist implements fn:apply yet or not, but it would be the easiest route.

dizzzz commented 6 years ago

for short term this solution looks ok to me; it has been requested before

fn:apply is not yet implemented yet I think...

craigberry commented 6 years ago

There is a commit at the tip of the xq31-templating branch that attempts to make it handle an arbitrary number of parameters, but it does use fn:apply, so maybe that's why it was never integrated.

joewiz commented 6 years ago

Wolfgang implemented fn:apply back in mid-2015. http://exist-db.org/exist/apps/fundocs/view.html?uri=http://www.w3.org/2005/xpath-functions#apply.2.

@craigberry Would you be able to rework your PR to use this? Do you need the xq31-templating branch to be merged before this is possible?

craigberry commented 6 years ago

I did a checkout of xq31-templating followed by "git rebase master." The one commit on that branch applied cleanly. I then built and tested with my app that has 16 template parameters. All is well. So I think the best way forward is probably to reject this PR and merge that branch (which I wasn't aware of when I initially made the PR). I don't know what versions of eXist the shared-resources package needs to run on, i.e., if it's still expected to be upgradeable by eXist installations from before fn:apply became available. If so, then this PR on its own branch somewhere might still be useful to someone.

craigberry commented 6 years ago

In my fork of this repository, I undid my previous commit and replaced it with Wolfgang's commit from the tip of xq31-templating using a force push. I hope that means this PR has now been updated to include the fn:apply approach. In any case, that is what I've been using in production for months now, and it would be nice to be able to stop maintaining my own fork.

adamretter commented 6 years ago

This looks okay to me, but I can't comment on if it actually works. Perhaps @wolfgangmm or @joewiz could also review?

joewiz commented 6 years ago

@craigberry Thanks for confirming that the xq31-templating branch works for you and your need for higher numbers of parameters. I've done as you suggested earlier and turned Wolfgang's original branch containing the fn:apply implementation and submitted a PR based on it, #29. As I mention there, we may need to ensure the package can only be installed by versions of eXist that support XQuery 3.1 and fn:apply. I'm hopefully we can get this into an upcoming release of shared-resources.