Skip to content

CODING_STANDARDS.md: add rules for bool/zend_result return types #10630

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

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

MaxKellermann
Copy link
Contributor

@nikic mentioned this guideline on php-internals
(https://news-web.php.net/php.internals/119573), but it appears to be undocumented.

Verified

This commit was signed with the committer’s verified signature.
mroeschke Matthew Roeschke
@nikic mentioned this guideline on php-internals
(https://news-web.php.net/php.internals/119573), but it appears to be
undocumented.
@devnexen
Copy link
Member

Thanks for this, the message itself is correct to me but let s see what other folks think of its formulation.

@devnexen devnexen requested a review from Girgias February 21, 2023 17:35
@Girgias
Copy link
Member

Girgias commented Feb 22, 2023

This looks reasonable, but sometime true/false being used for success/failure can also make sense (and is already done in some place so I wouldn't blindly convert stuff from bool to zend_result)

@Girgias Girgias removed their request for review February 22, 2023 14:02
@MaxKellermann
Copy link
Contributor Author

sometime true/false being used for success/failure can also make sense

That's how I usually do it, but @nikic disagreed. The rule added by this PR is not my opinion, but @nikic's. Whatever rule PHP has, it should be documented.

@Girgias Girgias merged commit da777d4 into php:master Feb 22, 2023
@MaxKellermann MaxKellermann deleted the coding_standards_zend_result branch February 22, 2023 14:19
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

3 participants