Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

125: array:partition #454

Closed

Conversation

michaelhkay
Copy link
Contributor

@michaelhkay michaelhkay commented Apr 24, 2023

This PR revisits array:partition, with extra editorial clarification of the spec; including but not confined to fixing issue #125.

I suggest we schedule this PR for discussion since we have not previously discussed it.

One question for the group is what the name of the function should be (including the choice of namespace).

Another is whether the polarity of the callback function should be changed (from break-when to continue-when or similar).

We could also consider returning an array of sequences rather than a sequence of arrays. (But in my view sequences of arrays are rather easier to manage at the moment.)

@ChristianGruen
Copy link
Contributor

One question for the group is what the name of the function should be (including the choice of namespace).

Now that you mention it, I think that fn:partition would be the best choice:

  • The current name suggests that the input argument is an array.
  • We shouldn’t use the array namespace just because a function returns an array,

If we want to stick with the array prefix, we could merge array:partition with array:build and introduce additional arguments (but it feels better to have fn:partition).

@ChristianGruen ChristianGruen changed the title Fix issue #125 - array:partition 125: array:partition May 2, 2023
@michaelhkay
Copy link
Contributor Author

I have made further changes to rename array:partition as fn:partition

<eg>fn:fold-left($input, (), function($partitions, $next) {
if (empty($partitions) or $break-when($partitions[last()]?*, $next))
then ($partitions, [$next])
else (fn:trunk($partitions), array{$partitions[last()], $next})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
else (fn:trunk($partitions), array{$partitions[last()], $next})
else (trunk($partitions), array{foot($partitions), $next})

@michaelhkay
Copy link
Contributor Author

I'm not sure about all these changes.

The return type: it's an sequence of arrays; each array has one or more members, and each member is a single item. I think that's correctly expressed as array(item())*. We can't express the constraint that the array has one or members, we can only express the constraint that each member is a single item.

Use of "fn:" prefix. In the past examples in the specs have always had a convention that calls to functions in the fn namespace are prefixed. It's a bore and I'd like to get rid of it (and it's sometimes honoured in the breach) but I think it should be done systematically if it's going to be done at all.

@ChristianGruen
Copy link
Contributor

I think that's correctly expressed as array(item())*.

So true. Sorry. About time to skip the morning hours again.

Use of "fn:" prefix. In the past examples in the specs have always had a convention that calls to functions in the fn namespace are prefixed.

Interesting: I spotted many examples that omitted the prefix (that’s even the case in the given pull request – see last(), count(), boolean(), sum(), etc.), so I assumed the convention is that the prefix shall exclusively be used for the function that’s presented. Good to know. And I agree, it’s probably better to sanitize this separately.

I think there’s just one example that would be better readable – if written as trunk($partitions), array{foot($partitions).

ChristianGruen added a commit to qt4cg/qt4tests that referenced this pull request May 7, 2023
ChristianGruen added a commit to qt4cg/qt4tests that referenced this pull request May 7, 2023
@ChristianGruen
Copy link
Contributor

@michaelhkay I’m afraid, it seems the PR needs to be fixed due to my cleanups.

@michaelhkay
Copy link
Contributor Author

I'm going to scrap this PR and raise a new one.

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.

None yet

2 participants