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

Document pop, shift, push and append more concisely #3973

Merged
merged 3 commits into from
Oct 18, 2021
Merged

Document pop, shift, push and append more concisely #3973

merged 3 commits into from
Oct 18, 2021

Conversation

dumarchie
Copy link
Collaborator

@dumarchie dumarchie commented Oct 17, 2021

The problem

In #3702 I included various signatures in "Defined as" routine introductions that were exact copies of those found in the Rakudo implementation. I now think that was not a good idea, because

  • some of the parameter names in these signatures are quite meaningless, and
  • some of the multi implementations exist only for performance reasons.

Consequently, these introductions did not provide a good gist of these routines.

Solution provided

I used more meaningful parameter names and removed multi signatures that are functionally irrelevant. Additionally, I edited the documentation of these routines where it referred to parameters or I just thought it could be improved.

In the "Defined as" sections, use meaningful parameter names and
omit multis that are implemented only for performance reasons.
This should more clearly convey the intent of these routines.
Copy link
Contributor

@JJ JJ left a comment

Choose a reason for hiding this comment

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

Text is great, change of names is great, elimination of sub/method signatures does not match the policy we usually follow here, I'm afraid. I'd appreciate if you left them the way they are, after checking that matches the actual implementation.

doc/Type/Array.pod6 Show resolved Hide resolved
and returns the modified array. If any argument is a C<Slip>, method C<push>
will add the values produced by the argument's L<iterator|/routine/iterator>.
It throws if the invocant array or a C<Slip> L<is lazy|/routine/is-lazy>.
Adds the provided values to the end of the array, and returns the modified
Copy link
Contributor

Choose a reason for hiding this comment

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

This explanation is clearly much clearer than the previous one; but that one could maybe be kept or moved to the end.

doc/Type/Array.pod6 Show resolved Hide resolved
doc/Type/independent-routines.pod6 Outdated Show resolved Hide resolved
doc/Type/independent-routines.pod6 Show resolved Hide resolved
doc/Type/independent-routines.pod6 Show resolved Hide resolved
Probably best thought of as a simple:

sub append(\a, +b)
multi sub append(\container, +values)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same reply, which also applies to pop and shift.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, not the usual policy to reinterpret the source, but to match it exactly to the extent it's possible.

@dumarchie
Copy link
Collaborator Author

As you may have understood from my other comments I question the policy of documenting all multi signatures found in the Rakudo implementation. However, I do understand that this PR is not the place to try and resolve disagreements about such policies, so I'll restore the documentation of multi signatures found in the Rakudo implementation. I'll put them in a logical order (most generic first) and I'll restore the "Probably best thought of as" signatures where the most generic signature does not convey the intent of the routine, i.e. in the documentation of append.

@JJ
Copy link
Contributor

JJ commented Oct 18, 2021

As you may have understood from my other comments I question the policy of documenting all multi signatures found in the Rakudo implementation. However, I do understand that this PR is not the place to try and resolve disagreements about such policies, so I'll restore the documentation of multi signatures found in the Rakudo implementation. I'll put them in a logical order (most generic first) and I'll restore the "Probably best thought of as" signatures where the most generic signature does not convey the intent of the routine, i.e. in the documentation of append.

Thanks, Peter. I really appreciate that.

@lizmat
Copy link
Collaborator

lizmat commented Oct 18, 2021

@dumarchie Re implementation of .append on native arrays: please note that the only thing you can put in a native array, are values of that type of native. Preserving structure therefore doesn't make sense, or should be an execution error.

@dumarchie
Copy link
Collaborator Author

@lizmat, don't you agree that .append on a native array should follow the single argument rule, just like .append on an Array?

In my opinion, this is correct:

> my int @a = 1, 2;
[1 2]
> my int @b = 3, 4;
[3 4]
> @a.append(@b);
[1 2 3 4]

But this is not:

> @a.append: $(@b);
[1 2 3 4 3 4]

The reason that the last call to .append does not throw is that the current implementation just flattens all arguments instead of following the single argument rule.

@lizmat
Copy link
Collaborator

lizmat commented Oct 18, 2021

Well, you could argue that the latter should throw, yes, as you can only represent native ints in a native int array.

@dumarchie
Copy link
Collaborator Author

That's not my point. In the example above, $(@b) is an itemized native int array. If you append an itemized array to an Array the argument is not flattened:

> my @a = 1, 2;
[1 2]
> my @b = 3, 4;
[3 4]
> @a.append: $(@b);
[1 2 [3 4]]

@lizmat
Copy link
Collaborator

lizmat commented Oct 18, 2021

But you can not push anything that's not an native int to a native int array. An itemized native int array is not a native int. So either that should throw, or DWIM. I guess we're arguing on whether DWIM isn't more of a WAT ?

@dumarchie
Copy link
Collaborator Author

Well, my idea of DWIM is that it should be possible to .append an itemized one-dimensional native int array to a two-dimensional native int array. And that makes the current behavior more of a WAT 😄

@dumarchie
Copy link
Collaborator Author

B.t.w. there's also an issue with .push on native arrays:

> my int @a = 1, 2;
[1 2]
> my int @b = 3, 4;
[3 4]
> @a.push: @b.Slip, @b.Slip;
[1 2 3 4 3 4]
> @a.push: @b.Slip;
Type check failed in push to int array; expected int but got Slip (slip(3, 4))
  in block <unit> at <unknown file> line 1

All multi signatures found in the Rakudo implementation should be
documented. Parameter names can be changed to fit the documentation.
Not everybody agreed this example was redundant.
Copy link
Contributor

@JJ JJ left a comment

Choose a reason for hiding this comment

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

Thanks a lot. I really appreciate the effort you've put into this (and previous versions of this page)

@JJ JJ merged commit b2fa218 into Raku:master Oct 18, 2021
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.

3 participants