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

Generate methodysnopses based on stubs for Zend functions and methods #168

Closed
wants to merge 5 commits into from

Conversation

kocsismate
Copy link
Member

@kocsismate kocsismate commented Oct 29, 2020

No description provided.

@cmb69
Copy link
Member

cmb69 commented Oct 29, 2020

Thank you for working on this! Highly appreciated.

@kocsismate kocsismate changed the title Replace methodsynopses based on PHP 8.0 stubs Generate methodysnopses based on stubs Oct 30, 2020
@kocsismate kocsismate force-pushed the generate-stub-poc branch 2 times, most recently from c915127 to a736d10 Compare November 2, 2020 08:54
@cmb69 cmb69 removed the variadics label Nov 2, 2020
@kocsismate kocsismate force-pushed the generate-stub-poc branch 2 times, most recently from 3515890 to 120a569 Compare November 7, 2020 23:43
@kocsismate kocsismate changed the title Generate methodysnopses based on stubs Generate methodysnopses based on stubs for Zend functions and methods Nov 7, 2020
@nikic
Copy link
Member

nikic commented Nov 9, 2020

I scrolled through this, and it looks good to me! Apart from the role="procedural", I didn't spot any errors. @cmb69 Any thoughts?

I think we might want to do a post-processing str_replace to preserve that newline before the final comment, to reduce the amount of spurious diff.

@kocsismate
Copy link
Member Author

I think we might want to do a post-processing str_replace to preserve that newline before the final comment, to reduce the amount of spurious diff.

I'll try to add this to the code.

@kocsismate
Copy link
Member Author

I think we might want to do a post-processing str_replace to preserve that newline before the final comment, to reduce the amount of spurious diff.

I'm afraid this is not that easy. Some pages do the opposite, and doesn't already have a newline before/after the root element tags, so if I tried str_replace(), I would add some new spurious diff. Arguably, much less than currently.

Do you insist on this change, or is it ok if these documents are normalized to get rid of the whitespaces in question?

@Girgias
Copy link
Member

Girgias commented Nov 10, 2020

I think we might want to do a post-processing str_replace to preserve that newline before the final comment, to reduce the amount of spurious diff.

I'm afraid this is not that easy. Some pages do the opposite, and doesn't already have a newline before/after the root element tags, so if I tried str_replace(), I would add some new spurious diff. Arguably, much less than currently.

Do you insist on this change, or is it ok if these documents are normalized to get rid of the whitespaces in question?

The issue is mostly with translations, as spurious diffs still forces translations to update the EN-Revision tag. So the less the better IMHO

EDIT: Just looked at the current diff, as it seems files already need to be updated the whitespace diffs can be ignored by translations

@kocsismate
Copy link
Member Author

Just looked at the current diff, as it seems files already need to be updated the whitespace diffs can be ignored by translations

Exactly! :)

@nikic
Copy link
Member

nikic commented Nov 12, 2020

Is this one ready to go now?

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Pretty good job, @kocsismate! :)

Whether we want to merge as is – well, how thorough do we like to be right now? I left some comments for consideration.

language/predefined/closure/bind.xml Show resolved Hide resolved
language/predefined/closure/construct.xml Show resolved Hide resolved
<methodparam choice="opt"><type>int</type><parameter>severity</parameter><initializer><constant>E_ERROR</constant></initializer></methodparam>
<methodparam choice="opt"><type class="union"><type>string</type><type>null</type></type><parameter>filename</parameter><initializer>&null;</initializer></methodparam>
<methodparam choice="opt"><type class="union"><type>int</type><type>null</type></type><parameter>line</parameter><initializer>&null;</initializer></methodparam>
<methodparam choice="opt"><type class="union"><type>Throwable</type><type>null</type></type><parameter>previous</parameter><initializer>&null;</initializer></methodparam>
Copy link
Member

Choose a reason for hiding this comment

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

Assuming these 3 params have not been nullable before 8.0, this would need to be documented (maybe even separate sysnopses for PHP 7/8?).

@nikic
Copy link
Member

nikic commented Nov 16, 2020

Whether we want to merge as is – well, how thorough do we like to be right now? I left some comments for consideration.

Please, let's not do any changes, apart from the automated signature changes and anything that is required to keep it building properly (the constructorsynopsis change in this case). We'll get completely bogged down otherwise.

@kocsismate
Copy link
Member Author

kocsismate commented Nov 24, 2020

@cmb69 Can you please add a label to this PR? Or alternatively, give me right to do so ^^

@kocsismate
Copy link
Member Author

I've just rebased to master, and added a few change log entries where doing so seemed necessary. I also noticed that the manual entries are yet to be added for Stringable and InternalIterator.

Copy link
Contributor

@sgolemon sgolemon left a comment

Choose a reason for hiding this comment

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

Generally okay with this (and appreciate the effort!), but in a few spots the parameter names are changed. Some of these are clearly for the better, some are questionable or even for the worse. We should probably update the stubs to make the names match documentation in most cases since the docs were (presumably) written by someone who put editorial thought into the name.

@kocsismate
Copy link
Member Author

kocsismate commented Mar 18, 2021

@sgolemon Together with Nikita we made quite a lot of effort to improve parameter names. :) Which ones are you not satisfied with?

Unless a parameter name is really broken, I don't think we can change them in the stubs because the arginfo structures are generated from the stubs, so we would introduce a BC break for named arguments.

@sgolemon
Copy link
Contributor

Which ones are you not satisfied with?

The one that stood out to me was handle -> resource. But there are some like class_name to class which I found a little suspect. resource and class are types and I have a small worry that they conflate themselves by default.

Unless a parameter name is really broken, I don't think we can change them in the stubs because the arginfo structures are generated from the stubs, so we would introduce a BC break for named arguments.

Argh... good point. Fine... Leave as-is then. :(

@kocsismate
Copy link
Member Author

I know it's a large one.. But it would be very useful for users. Can you please review it, @cmb69 @kamil-tekiela ?

@cmb69 cmb69 self-assigned this May 22, 2021
@cmb69 cmb69 closed this in c44475e May 22, 2021
@cmb69
Copy link
Member

cmb69 commented May 22, 2021

Thanks everybody!

@kocsismate kocsismate deleted the generate-stub-poc branch May 23, 2021 13:48
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.

5 participants