Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add callable template support #199

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
mvorisek wants to merge 1 commit into phpstan:1.23.x from mvorisek:parse_generic_callable
Closed

Add callable template support #199

mvorisek wants to merge 1 commit into phpstan:1.23.x from mvorisek:parse_generic_callable

Conversation

Copy link
Contributor

@mvorisek mvorisek commented Jun 24, 2023
edited
Loading

mad-briller reacted with thumbs up emoji
@mvorisek mvorisek changed the title (削除) Parse generic callable template (削除ここまで) (追記) Parse callable template (追記ここまで) Jun 24, 2023
@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
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!

mvorisek reacted with heart emoji
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.

Copy link
Contributor Author

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

Wirone reacted with eyes emoji

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."

@ondrejmirtes ondrejmirtes changed the base branch from 1.22.x to 1.23.x September 18, 2023 12:26
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?

$tokens->tryConsumeTokenType(Lexer::TOKEN_PHPDOC_EOL);

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

@ondrejmirtes ondrejmirtes Oct 29, 2023

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

@jiripudil jiripudil Nov 13, 2023

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

@ondrejmirtes ondrejmirtes Nov 13, 2023

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

@mad-briller mad-briller Feb 23, 2024

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

@ondrejmirtes ondrejmirtes Feb 23, 2024

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 :)

mad-briller reacted with thumbs up emoji
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.

Copy link
Contributor Author

Landed thru #232, thanks everyone involved!

@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

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

+2 more reviewers

@jiripudil jiripudil jiripudil left review comments

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

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /