Skip to content

Update ext/com_dotnet parameter names #6296

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

Closed
wants to merge 3 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Oct 7, 2020

Sorry, I lost track of the current parameter name conventions, so this is supposed to serve as base for further improvements/fixes.

PS: this PR should be added to php/php-tasks#23.

Copy link
Member

@kocsismate kocsismate left a comment

Choose a reason for hiding this comment

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

There's an $lcid parameter in the beginning. Unfortunately, I can't suggest any better name, even after reading the documentation. Can you please change it too? :)

@cmb69
Copy link
Member Author

cmb69 commented Oct 8, 2020

Thanks for reviewing, @kocsismate! I'm having a hard time with $lcid as well, but given that GetSystemDefaultLCID() speaks of "locale identifier" (although LCID means language code identifier), I went with $locale_id.

@kocsismate
Copy link
Member

Looks good to me!

@nikic
Copy link
Member

nikic commented Oct 8, 2020

There's a couple of functions like...

function variant_abs(mixed $left): variant {}

where the use of $left seems pretty odd. https://www.php.net/variant_abs uses $val, so maybe $value would be a good choice for these functions?

@cmb69
Copy link
Member Author

cmb69 commented Oct 8, 2020

Indeed. Thanks @nikic!

@php-pulls php-pulls closed this in 561b581 Oct 8, 2020
@cmb69 cmb69 deleted the cmb/com-param-names branch October 8, 2020 09:46
@kocsismate
Copy link
Member

kocsismate commented Oct 9, 2020

While working on intl stuff, I've noticed that there's a couple of functions where the $left/$right terminology remained. I.e.: function variant_add(mixed $left, mixed $right): variant {}

Could (or should) these be changed as well? To $value1 and $value2 for example.

@nikic
Copy link
Member

nikic commented Oct 9, 2020

@kocsismate left/right makes sense for the binary operators, so I'm okay either way there. We do call these $num1/$num2 in ext/gmp and ext/bcmath as well though, so maybe $value1/$value2 would be more consistent than $left/$right.

@kocsismate
Copy link
Member

ere. We do call these $num1/$num2 in ext/gmp and ext/bcmath as well though, so maybe $value1/$value2 would be more consistent than $left/$right.

Yeah, that's the only reason why I brought up this. :)

@cmb69
Copy link
Member Author

cmb69 commented Oct 9, 2020

Since both "values" are variants, I'd thought about $var1/$var2, but that would be confusing ("variable"). $value1/$value2 is fine for me, though.

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.

3 participants