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

Clean up array function signatures for named parameters #6046

Closed
wants to merge 6 commits into from

Conversation

Crell
Copy link
Contributor

@Crell Crell commented Aug 27, 2020

Folding a few other PRs into this one... After some discussion in chat, here's the guidelines I used:

  • The array "subject" of a function gets called $array.
  • Further parameters should be self-descriptive if used as a named parameter, and a full word, not an abbreviation.
  • If there is a "bunch more arrays" variadic, it gets called $arrays (because that's what was already there).
  • A few functions have a variadic "a bunch more arrays, and then a callable", and were already called $rest. I left those as is and died a little inside.
  • Any callable provided to an array function that acts on the array is called $callback. (Nearly all were already, I just fixed the one or two outliers.)
  • array_multisort() is beyond help so I ran screaming.

I think everything else in the array_*() and sort*() namespace makes sense now.


function array_replace_recursive(array $array1, array ...$arrays): array {}
function array_replace_recursive(array $array, array ...$substitutions): array {}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about $replacements? It would be consistent with array_splice() and heading towards consistency with many other $replace parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking through the stub file, $replacement is only used once, in array_splice(). $replace is used a whole bunch of times. $substitutions is used nowhere but in this change. So, yeah, $replacements makes sense.

I think we should then also change array_splice() to $replacements. It's not variadic, but it is an array.

@Crell Crell force-pushed the array-funcs branch 4 times, most recently from 7d1537d to b7db46c Compare September 8, 2020 14:43
@Crell
Copy link
Contributor Author

Crell commented Sep 8, 2020

Rebased against master and squashed. 🤞

@Crell Crell force-pushed the array-funcs branch 2 times, most recently from a2db922 to 3bdcbc0 Compare September 9, 2020 21:50
Change $sort_flags  to $flags.

Remove $array1 from array_map()

Rename $cmp_function to $comparison in user-sorting functions.

Change $funcname to the more standard $callback.

Remove $array1 from array_replace*().

Simplify array_keys() arguments to be more usefully named.

Change array_diff*() and array_intersect*() to have be more logically named parameters.

Standardize array_key_exists() on $array, which the docs already use.

Switch array_fill*() arguments to be more self-descriptive.

Change $vars to $values in array_unshift(), to be more consistent and descriptive.

Rename $userdata to $arguments, since that is how it will be used.

Change array_replace() to use $substitutions instead.

Update more tests.

Standardize callback name to $callback.

Update tests.

Use plurals for variadics.

Update another test.

Update reflection tests.

Fix another test.

Use $replacements for substitution values.

Update tests.
@Crell
Copy link
Contributor Author

Crell commented Sep 11, 2020

Apparently #6097 stepped all over this one, and the diff/intersect functions are now incompatible with named parameters. 😢 For lack of better options or feedback I rebased it and stripped out those functions. I disagree with that change but I don't want to block the rest of these.

The AppVeyor fails seem entirely unrelated. I'm not sure what's going on there but it may be an AppVeyor problem.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Minor nit, but seems reasonable otherwise.

@php-pulls
Copy link

Comment on behalf of pollita at php.net:

Pushed as 96f2f31

@Crell Crell deleted the array-funcs branch September 14, 2020 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants