Skip to content

Remove proto comments #5758

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 2 commits into from
Closed

Remove proto comments #5758

wants to merge 2 commits into from

Conversation

MaxSem
Copy link
Contributor

@MaxSem MaxSem commented Jun 23, 2020

They've been made obsolete by .stub.php files and are often outdated as they're not the source of truth.

@kocsismate
Copy link
Member

kocsismate commented Jun 23, 2020

@MaxSem Please see #5532

Unfortunately, removing folding marks wasn't welcomed by a few people. :( But I think you could remove protos instead. I'd be happy if you wanted to take this task over from me. :)

@MaxSem MaxSem changed the title Remove {{{ vim folding }}} Remove proto comments Jun 23, 2020
@MaxSem
Copy link
Contributor Author

MaxSem commented Jun 23, 2020

@kocsismate, better now? :)

@kocsismate
Copy link
Member

@MaxSem Really nice! :) I had a quick look, and the PR mostly looked good to me!

The only very small gotcha I saw was that the description is repeated in case of functions that have multiple aliases (e.g. in intl). I don't think you have to address these now, so I'm perfectly ok with keeping them for now.

In the meanwhile, I'll convert my PR to remove the protos from tests as well.

/* {{{ proto int collator_get_attribute( Collator $coll, int $attr )
* Get collation attribute value.
/* {{{ * Get collation attribute value. }}} */
/* {{{ * Get collation attribute value.
Copy link
Member

Choose a reason for hiding this comment

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

We should try to avoid these odd cases at least.

@MaxSem MaxSem force-pushed the braces branch 2 times, most recently from 649c2ed to 960589c Compare July 1, 2020 13:33
@kocsismate
Copy link
Member

@MaxSem This looks even better! I only saw a handful of duplicated comments (e.g.: zif_locale_set_default()). If @nikic approves it, I'll merge the PR, and then I'll fixup those small number of glitches.

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Based on a quick scroll through this looks mostly fine. One recurring issue that should be fixed before merging is that /* {{{ * should be just /* {{{. Other minor issues can be fixed up after the fact, to avoid bit rot.

@@ -103,8 +103,7 @@ static void php_converter_default_callback(zval *return_value, zval *zobj, zend_
}
/* }}} */

/* {{{ proto void UConverter::toUCallback(int $reason,
string $source, string $codeUnits,
/* {{{ string $source, string $codeUnits,
int &$error) */
Copy link
Member

Choose a reason for hiding this comment

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

These aren't right.

MaxSem added 2 commits July 6, 2020 21:18
They've been made obsolete by .stub.php files and are often
outdated as they're not the source of truth.
@kocsismate
Copy link
Member

I had a look at the new commit, and it looked good. So I took the liberty and merged this in order not to face any conflicts. @nikic If you find any formatting issues, I can also help sort them out.

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