Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

1725b Further elaboration of duplicates handling in maps #1740

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

michaelhkay
Copy link
Contributor

Actions QT4CG-107-02 and QT4CG-107-03.

The three functions map:build, map:of-pairs, and map:merge now all have the same options parameters, and avoid duplication in the specification. The xsl:map instruction is defined by reference to map:merge.

Although the action suggested specifying these functions to use the first key from a set of duplicates, I found this was not possible because of the way map:put is defined. They therefore use the last key from the set of duplicates.

Fix #1725

@michaelhkay michaelhkay added Enhancement A change or improvement to an existing feature Tests Needed Tests need to be written or merged labels Jan 29, 2025
@ChristianGruen
Copy link
Contributor

As discussed on the mailing list, the decision to use the old or the new key should be made implementation-dependent again, as it had originally been proposed in #1727 (sorry for the additional work).

To speak in examples, it will be up to the processor to return { 1: () } or { 1e0: () } for the following expressions:

map:build((1, 1e0), fn { . }, fn { () }),
map:merge(({ 1: () }, { 1e0: () })),
map:put({ 1: () }, 1e0, ())

@michaelhkay michaelhkay added Tests Added Tests have been added to the test suites XQFO An issue related to Functions and Operators and removed Tests Needed Tests need to be written or merged labels Jan 29, 2025
ChristianGruen added a commit to qt4cg/qt4tests that referenced this pull request Jan 30, 2025
@ChristianGruen
Copy link
Contributor

Preliminary feedback (from a tester): Great to be able to use a function for map:merge. Maybe combine and duplicates could be combined…

map:merge(
  ({ 1: 2 }, { 1: 3 }),
  'duplicates': op('*')
)

…as it is not easy to distinguish between them.

ChristianGruen added a commit to qt4cg/qt4tests that referenced this pull request Jan 30, 2025
@ChristianGruen
Copy link
Contributor

I hope my commits to the test suite to accept both the old and the new key in updated map entries were not too hasty.

ChristianGruen added a commit to qt4cg/qt4tests that referenced this pull request Jan 30, 2025
@michaelhkay
Copy link
Contributor Author

I have combined the "duplicates" and "combine" options - a great improvement, thanks for the suggestion.

I haven't yet implemented the change to make the choice of keys implementation-dependent, I felt this might need more discussion. In particular, do you want this to apply to map:put()? I feel that map:put() should definitely use the new key, and this seems to lead naturally to map:merge() etc using the last of the duplicate keys.

@ChristianGruen
Copy link
Contributor

In particular, do you want this to apply to map:put()?

Definitely yes. In mutable maps, the key serves just for lookup. It would be rather unorthodox to change the key of a map whenever a map entry is updated, and I have never quite understood why it was defined that way for map:put. Now that we are redefining maps, it would be a good opportunity to let the implementation decide, and it will remain an egde case anyway.

From the (secondary) implementation perspective, I have implemented all variants, and updating only the key would be the most efficient solution as it does not change the key order.

ChristianGruen added a commit to qt4cg/qt4tests that referenced this pull request Jan 31, 2025
@michaelhkay
Copy link
Contributor Author

michaelhkay commented Feb 4, 2025

Further updates:

  • map:put, may now retain the old key instead of having to use the new key
  • consequent changes to map:merge, map:build, map:of-pairs in their duplicates handling
  • xsl:map now aligned with map:of-pairs, attribute renamed from on-duplicates to duplicates, schema updated.

Reinstated the "tests needed" label since XSLT tests need to be updated with these latest changes.

@michaelhkay michaelhkay added Tests Needed Tests need to be written or merged and removed Tests Added Tests have been added to the test suites labels Feb 4, 2025
@ChristianGruen
Copy link
Contributor

  • map:put, may now retain the old key instead of having to use the new key

The XPath Data Model rules may still need to be tweaked to reflect this freedom. It currently says:

The key/value pairs in the returned map are as follows:

  • One key/value pair for every key/value pair present in $map whose key is not equal to $key; plus
  • One key/value pair whose key is $key and whose associated value is $value.

The entry order in the returned map reflects the entry order in the supplied $map. If the key of the new entry was present in $map then the new entry replaces that entry retaining its current position; otherwise, the new entry is added after all existing entries.

@michaelhkay michaelhkay force-pushed the 1725b-rules-for-duplicates-in-maps branch from 39fa65b to 74e5b03 Compare February 5, 2025 00:02
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Enhancement A change or improvement to an existing feature Tests Needed Tests need to be written or merged XQFO An issue related to Functions and Operators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Position of duplicates in ordered maps
2 participants