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.

Sign up for GitHub

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

Jump to bottom

Add callable template support #199

Closed
wants to merge 1 commit into from
Closed

Add callable template support #199

wants to merge 1 commit into from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Jun 24, 2023 •
edited
Loading

@mvorisek mvorisek force-pushed the parse_generic_callable branch 6 times, most recently from 10f5ff4 to 4d2da12 Compare June 24, 2023 19:43
@mvorisek mvorisek mentioned this pull request Jun 24, 2023
Merged
@mvorisek mvorisek force-pushed the parse_generic_callable branch 2 times, most recently from c86cc46 to 5e50098 Compare June 24, 2023 21:31
@mvorisek mvorisek changed the title Parse generic callable template Parse callable template Jun 24, 2023
@mvorisek mvorisek force-pushed the parse_generic_callable branch 3 times, most recently from 0f3080a to e0f5e39 Compare June 25, 2023 07:58
@mvorisek mvorisek marked this pull request as ready for review June 25, 2023 07:58
@mvorisek mvorisek changed the title Parse callable template Add callable template support Jun 25, 2023
@mvorisek mvorisek force-pushed the parse_generic_callable branch 4 times, most recently from 2ed04fa to 03753eb Compare June 26, 2023 09:31
jiripudil
jiripudil reviewed Jun 26, 2023
View reviewed changes
Copy link
Contributor

@jiripudil jiripudil left a comment

Choose a reason for hiding this comment

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

Hi, I took a look since this somehow ended up in my notifications and piqued my curiosity. But I'm really just passing by and only wanted to point out a few topics for discussion, so don't take my comments too seriously :)

Nice work overall!

src/Ast/Type/CallableTypeTemplateNode.php Outdated Show resolved Hide resolved
src/Parser/TypeParser.php Outdated Show resolved Hide resolved
src/Parser/TypeParser.php Show resolved Hide resolved
@mvorisek mvorisek force-pushed the parse_generic_callable branch 2 times, most recently from 891d9ce to 02177ca Compare June 26, 2023 12:06
@mvorisek
Copy link
Contributor Author

mvorisek commented Jul 4, 2023

@ondrejmirtes this parser PR should be fully done, phpstan/phpstan#8964 (comment) might be easy for you, but I am not familiar with phpstan internals so I would appreciate a help with the integration into phpstan. Thank you.

@mvorisek
Copy link
Contributor Author

@ondrejmirtes this PR is here for over a month, may I ask you to review it?

@mvorisek mvorisek mentioned this pull request Sep 1, 2023
Open
ondrejmirtes
ondrejmirtes requested changes Sep 18, 2023
View reviewed changes
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

These are the two main issues I see in this PR.

Also I find the changes of the parser hard to understand, I have to inspect them in depth to understand the implications. Maybe if you could do the changes to the TypeParser in multiple smaller steps, I'd welcome them as separate PRs that I would merge before this one.

The credo to live by is: "Make the change easy, then make the easy change."

src/Printer/Printer.php Outdated Show resolved Hide resolved
src/Printer/Printer.php Show resolved Hide resolved
@mvorisek mvorisek mentioned this pull request Sep 18, 2023
Merged
@ondrejmirtes ondrejmirtes changed the base branch from 1.22.x to 1.23.x September 18, 2023 12:26
@mvorisek mvorisek force-pushed the parse_generic_callable branch 2 times, most recently from 127e2aa to 1c718de Compare October 1, 2023 15:50
@mvorisek
Copy link
Contributor Author

@ondrejmirtes sorry to tagging you again, can I please ask you to merge this PR if there is no feedback left?

ondrejmirtes
ondrejmirtes requested changes Oct 29, 2023
View reviewed changes
src/Parser/TypeParser.php
$tokens->tryConsumeTokenType(Lexer::TOKEN_PHPDOC_EOL);

// trailing comma case
if (!$isFirst && $tokens->isCurrentTokenType(Lexer::TOKEN_CLOSE_ANGLE_BRACKET)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why is there so much repeated code. We're already parsing template types in similar position for @method, there has to be a way to reuse that code: #160

Everything you should need is already in parseTemplateTagValue.

Copy link
Contributor Author

@mvorisek mvorisek Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can you please guide me how and what to reuse? The code in #160 (Parser/PhpDocParser in general) is for phpdoc instead of for type (Parser/TypeParser).

If the newly introduced Ast/Type/CallableTypeTemplateNode class is wanted (and it probably is - callable template should not consist of anything like description), then the code cannot be simplified much. As the grammar is not completely the same, basically the only parsing part to deduplicate would be <, ,, of.

If you want, feel free to push any changes to my branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think #160 implements the very same use case as this PR: it parses inline template tag definitions, disallowing descriptions, but accounting for everything else (bound types, defaults, ...). Perhaps it could suffice to extract the parseTemplateTagValue method into a TemplateTagParser that could be used in TypeParser as well?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that logic should be extracted so that it's usable by both TypeParser and PhpDocParser.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mvorisek are you going to look into this feedback or should i have a go at it?

Copy link
Member

Choose a reason for hiding this comment

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

@mad-briller Please have a go at it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mad-briller would you like to join the forces? Feel free to either reuse this PR as a separate one or submit a PR againts https://github.com/mvorisek/phpdoc-parser/tree/parse_generic_callable - this PR should be basically done, but the code should be deduplicated per #199 (comment) request.

@jiripudil jiripudil mentioned this pull request Feb 18, 2024
Draft
@ondrejmirtes ondrejmirtes mentioned this pull request Feb 23, 2024
Merged
@mad-briller mad-briller mentioned this pull request Feb 23, 2024
Merged
@mvorisek
Copy link
Contributor Author

Landed thru #232, thanks everyone involved!

@mvorisek mvorisek closed this Feb 23, 2024
@mvorisek mvorisek deleted the parse_generic_callable branch February 23, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mad-briller mad-briller mad-briller left review comments

@jiripudil jiripudil jiripudil left review comments

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mvorisek @ondrejmirtes @jiripudil @mad-briller