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

PSR-5 @inheritDoc support #2619

Merged
merged 1 commit into from Oct 9, 2020

Conversation

julienfalque
Copy link
Member

This PR adds support for PSR-5 explicit inheritance tag: the tag can be a regular tag instead of an inline one, and uses a capital "D".

@SpacePossum
Copy link
Contributor

SpacePossum commented Mar 20, 2017

👍 for the idea, please fix the tests :)
missing code sample for configuration option in the description I think

README.rst Outdated Show resolved Hide resolved
@SpacePossum
Copy link
Contributor

maybe it would be better to have a fixer for the casing of PHPDoc annotations and one fixer (this one) to deal with inline tags?

@julienfalque
Copy link
Member Author

It would be better, but I think that would still require an option in phpdoc_inline_tag to tweak its behavior, so maybe not worth it? Maybe better to rename the current fixer instead?

@SpacePossum
Copy link
Contributor

After the split up you would only need the configuration option here to (not) always make the inheritdoc inline. Like to hear more opinions!

@keradus
Copy link
Member

keradus commented Mar 27, 2017

What about:

  1. new fixer GeneralPhpdocTagRenameFixer
    Fixer that will rename inline-tag or annotation-tag based on configuration:
[
    'fix_annotation' => boolean,
    'fix_inline' => boolean,
    'ignore_casing' => boolean,
    'replacements' => [ 'inheritdocS' => 'inheritdoc', ...]
]
  1. new fixer PhpdocTagCasingFixer
    that will be proxy fixer to GeneralPhpdocTagRenameFixer that will fix most common tags

  2. new fixer PhpdocInlineTagNormalizerFixer
    that will:

- @{{   tag description?      }}}}}}
+ {@tag description?}

- {{   @tag description?      }}}}}}
+ {@tag description?}
  1. current PhpdocInlineTagFixer
  • shall reuse fixers 1 and 3 without extra logic
  • shall be deprecated

That way, you could achieve initial goal in very generic way: have @inheridDoc annotation tag

@SpacePossum
Copy link
Contributor

sounds good to me 👍

@julienfalque
Copy link
Member Author

Sounds like a good idea. Could you elaborate what the options would do exactly?

@keradus
Copy link
Member

keradus commented Mar 28, 2017

[
    'fix_annotation' => boolean,
    'fix_inline' => boolean,
    'ignore_casing' => boolean,
    'replacements' => [ 'inheritdocS' => 'inheritdoc', ...]
]
  • replacements - providing a replacement map
  • ignore_casing - if true the replaced tag may be written with different casing than provided in replacements, eg iNhErItDoCs would also be modified
  • fix_inline - rename inline tags, like in * Foo @{inheritdocs} because...
  • fix_annotation - rename annotations, like in * @inheritdocs because....

@keradus
Copy link
Member

keradus commented Sep 2, 2017

ping @julienfalque

@julienfalque
Copy link
Member Author

PR reworked from scratch.

README.rst Outdated Show resolved Hide resolved
@julienfalque julienfalque force-pushed the psr-5-inheritdoc branch 2 times, most recently from 236ba28 to ea7cf89 Compare September 18, 2017 21:59
README.rst Outdated Show resolved Hide resolved
@keradus
Copy link
Member

keradus commented Sep 20, 2017

This PR is touching 25 files, including already existing rules and few generic files. For that, it's hard to focus on end-change we want to achieve here. Having all changes in one commit is not helping as well.
I think we shall split this PR. As first step I see extracting ability for ProxyFixer to proxy multiple fixers at once. WDYT ?

@julienfalque julienfalque force-pushed the psr-5-inheritdoc branch 2 times, most recently from e2e5b61 to 78406b5 Compare October 8, 2017 10:47
@SpacePossum
Copy link
Contributor

@julienfalque can you split us the PR? I think a new fixer like the PhpdocTagCasingFixer can be reviewed on its own for example?

@julienfalque
Copy link
Member Author

I extracted support for multiple proxied fixers in AbstractProxyFixer: #3149.

keradus added a commit that referenced this pull request Oct 13, 2017
…ienfalque)

This PR was squashed before being merged into the 2.8-dev branch (closes #3149).

Discussion
----------

AbstractProxyFixer - Support multiple proxied fixers

Extracted from #2619.

Commits
-------

5f9f007 AbstractProxyFixer - Support multiple proxied fixers
@keradus
Copy link
Member

keradus commented Oct 13, 2017

extracted PR got merged

@julienfalque
Copy link
Member Author

Rebased.

@keradus
Copy link
Member

keradus commented Oct 14, 2017

I like the idea of @SpacePossum to split this PR down to ease with review and overall process.
Do you think there is still some nice thing that could be extracted?

@julienfalque
Copy link
Member Author

Maybe, but I'd like to address #2619 (comment) first.

@julienfalque julienfalque force-pushed the psr-5-inheritdoc branch 2 times, most recently from afb666e to cf19fe9 Compare October 16, 2017 21:43
@julienfalque julienfalque removed the WIP label Oct 16, 2017
@julienfalque
Copy link
Member Author

This PR should be ready for a new review iteration. Not sure there is more that can be extracted into a separate PR though.

@GrahamCampbell
Copy link
Contributor

What's the status of this PR?

@julienfalque
Copy link
Member Author

julienfalque commented Oct 26, 2019

This PR still needs some review. It's not a priority to me anymore as I tend to remove @inheritDocs now but I'd still be happy to get it merged :)

@SpacePossum SpacePossum added the RTM Ready To Merge label Oct 9, 2020
@SpacePossum
Copy link
Contributor

Thank you @julienfalque.

@SpacePossum SpacePossum removed the RTM Ready To Merge label Oct 9, 2020
@SpacePossum SpacePossum merged commit d522e31 into PHP-CS-Fixer:master Oct 9, 2020
@julienfalque julienfalque deleted the psr-5-inheritdoc branch October 9, 2020 10:13
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.

None yet

5 participants