Movatterモバイル変換

[0]ホーム
URL:

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

[FrameworkBundle] decouple debug:autowiring from phpdocumentor/reflection-docblock#29569

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:4.2fromSerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions
Dec 17, 2018
Merged

[FrameworkBundle] decouple debug:autowiring from phpdocumentor/reflection-docblock#29569

nicolas-grekas merged 1 commit intosymfony:4.2fromSerkanYildiz:add-fallback-debug-autowiring-command-class-descriptions
Dec 17, 2018

Conversation

@SerkanYildiz
Copy link
Contributor

@SerkanYildizSerkanYildiz commentedDec 11, 2018•
edited
Loading

QA
Branch?4.2
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets/
LicenseMIT

When phpDocumentor isn't installed thedebug:autowiring returns an empty string for class description. With this fallback the end user will get a description even phpDocumentor isn't installed.

Edit: Usage of phpDocumentor is completely removed from debug:autowiring command.

mickaelandrieu reacted with thumbs up emoji
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedDec 11, 2018•
edited
Loading

Thanks for submitting! During the SymfonyCon hackday last Saturday, someone worked exactly on this - except that they opened no PR so the code might be lost :)

Here are my suggestions:

  • consider this requirement for phpDocumentor as a bug - it's really broken to rely on it, DX wise
  • thus, rebase/retarget this for 4.2
  • let's completely remove the code path that uses phpDocumentor and parse inline ourselves - that's simple enough.

@SerkanYildiz
Copy link
ContributorAuthor

Hi@nicolas-grekas,

It was me during the SymfonyCon who started with this patch. Will apply your comments and update this PR. Thanks for your review!

@SerkanYildizSerkanYildiz changed the base branch frommaster to4.2December 11, 2018 16:00
@SerkanYildizSerkanYildiz changed the base branch from4.2 tomasterDecember 11, 2018 16:00
@SerkanYildizSerkanYildiz changed the base branch frommaster to4.2December 11, 2018 16:10
$summary =str_replace(
array('/','*',"\t","\r\n","\n","\r",''),
'',
substr($docComment,0,strpos($docComment,'@'))
Copy link
Contributor

@ro0NLro0NLDec 11, 2018•
edited
Loading

Choose a reason for hiding this comment

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

this seems very fragile (perhaps* @ works better). But tend to prefer an explicit regex to capture the short description as a sentence.

This captures both short+long description and removes newlines in between.

Copy link
ContributorAuthor

@SerkanYildizSerkanYildizDec 12, 2018•
edited
Loading

Choose a reason for hiding this comment

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

@ro0NL Can you help me with a regex to get the description? Would be great!

Copy link
Contributor

Choose a reason for hiding this comment

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

@SerkanYildiz something likehttps://regex101.com/r/Sqcmmf/1/

It matches the first sentence perhttps://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md#51-summary

A Summary MUST end with two sequential line breaks, unless it is the only content in the PHPDoc.

The current regex does not include single new lines, e.g.short line 1\nshort line 2 would only matchshort line 1. AFAIK it's OK.

->getSummary();
}
}catch (\ReflectionException |\InvalidArgumentException$e) {
$docComment =$r->getDocComment();

Choose a reason for hiding this comment

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

if ($docComment = $r->getDocComment()) { should be kept imho

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

re-added the if statement thanks@nicolas-grekas

@SerkanYildiz
Copy link
ContributorAuthor

@ro0NL Your regex worked and made the tests pass (at least locally.) Thank You!

ro0NL reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to the4.2 milestoneDec 12, 2018
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

could you please add tests with several variants of class descriptions (or missing ones)?

@SerkanYildiz
Copy link
ContributorAuthor

@nicolas-grekas I don't now if it's necessary to add more tests. There were existing tests for theold implementation. We just changed the logic and IMO we just had to ensure that the tests should still pass

@ro0NL
Copy link
Contributor

we should add a test for

/** * line1 * line2 * * line 4 */

resulting inline1 line2

and

/** *oops * * @annot */

resulting inoops

@SerkanYildiz
Copy link
ContributorAuthor

@nicolas-grekas failing test is not related to this PR

Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

i think we're there :) last minor comments from me.

@SerkanYildiz
Copy link
ContributorAuthor

ping@nicolas-grekas , failing AppVeyor seems unrelated. Can we retrigger AppVeyor build?

@nicolas-grekasnicolas-grekas changed the title[FrameworkBundle] Implement fallback for class descriptor in debug:autowiring[FrameworkBundle] decouple debug:autowiring from phpdocumentor/reflection-docblockDec 17, 2018
@nicolas-grekas
Copy link
Member

Thank you@SerkanYildiz.

SerkanYildiz reacted with hooray emoji

@nicolas-grekasnicolas-grekas merged commit485ed4d intosymfony:4.2Dec 17, 2018
nicolas-grekas added a commit that referenced this pull requestDec 17, 2018
…ntor/reflection-docblock (SerkanYildiz)This PR was merged into the 4.2 branch.Discussion----------[FrameworkBundle] decouple debug:autowiring from phpdocumentor/reflection-docblock| Q             | A| ------------- | ---| Branch?       | 4.2| Bug fix?      | yesNew feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | /| License       | MITWhen phpDocumentor isn't installed the *debug:autowiring* returns an empty string for class description. With this fallback the end user will get a description even phpDocumentor isn't installed.**Edit:** Usage of phpDocumentor is completely removed from debug:autowiring command.Commits-------485ed4d [FrameworkBundle] decouple debug:autowiring from phpdocumentor/reflection-docblock
@SerkanYildizSerkanYildiz deleted the add-fallback-debug-autowiring-command-class-descriptions branchDecember 17, 2018 14:34
@fabpotfabpot mentioned this pull requestJan 6, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@dunglasdunglasAwaiting requested review from dunglas

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

@xabbuhxabbuhAwaiting requested review from xabbuh

+1 more reviewer

@ro0NLro0NLro0NL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

4 participants

@SerkanYildiz@nicolas-grekas@ro0NL@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp